UniValue does not have a constructor for enum values, however the
compiler will decay the enum into an int and select that constructor.
Avoid this compiler magic and clarify the code by explicitly selecting
the int-constructor.
This is needed for the next commit.
fa475e9c79 refactor: Return BResult from restoreWallet (MacroFake)
fa8de09edc Prepare BResult for non-copyable types (MacroFake)
Pull request description:
This avoids the `error` in-out param (and if `warnings` is added to `BResult`, it will avoid passing that in-out param as well).
Also, as it is needed for this change, prepare `BResult` for non-copyable types.
ACKs for top commit:
w0xlt:
reACK fa475e9c79
ryanofsky:
Code review ACK fa475e9c79. Changes since last review were replacing auto with explicit type and splitting commits
Tree-SHA512: 46350883572f13721ddd198f5dfb88d2fa58ebcbda416f74da3563ea15c920fb1e6ff30558526a4ac91c36c21e6afe27751a4e51b7b8bcbcbe805209f4e9014b
613e221149 test: remove unnecessary parens (Suhas Daftuar)
e939cf2b76 Remove atomic for m_last_getheaders_timestamp (Suhas Daftuar)
Pull request description:
Eliminate the unnecessary atomic guarding `m_last_getheaders_timestamp`, which is only accessed in a single thread (thanks to MarcoFalke for pointing this out).
Also address a nit that came up in #25454.
ACKs for top commit:
MarcoFalke:
review ACK 613e221149
vasild:
ACK 613e221149
Tree-SHA512: 6d6c473735b450b8ad43aae5cf16ed419154d72f4a05c0a6ce6f26caecab9db2361041398b70bf9395611c107d50897f501fa5fdbabb2891144bbc2b479dfdad
faace13b71 test: Remove duplicate MAX_BIP125_RBF_SEQUENCE constant (MacroFake)
fa0404dbb7 scripted-diff: [test] Rename BIP125_SEQUENCE_NUMBER to MAX_BIP125_RBF_SEQUENCE (MacroFake)
Pull request description:
Not sure why the python constant is named differently than the constant in the C++ source code.
Especially, if we use this in context of MAX+1 (https://github.com/bitcoin/bitcoin/pull/25575#discussion_r918569813) the rename makes sense, in my eyes.
ACKs for top commit:
theStack:
Code-review ACK faace13b71
glozow:
concept ACK faace13b71
Tree-SHA512: 1dc8cd0f067717f6ace8121b660e99f2d0f8c485c0d61b80413e07e7830e7dfaf9ce2c922c63ba2c6b42e805d59d88cd0d9c80a4b4a2fca47e77a3aba6cd4ec6
fa8a1c0696 rpc: Fix Univalue push_backV OOM in listtransactions (MacroFake)
Pull request description:
Related to, but not intended as a fix for #25229.
Currently the RPC will have the same data stored thrice:
* `UniValue ret` (memory filled by `ListTransactions`)
* `std::vector<UniValue> vec` (constructed by calling `push_backV`)
* `UniValue result` (the actual result, memory filled by `push_backV`)
Fix this by filling the memory only once:
* `std::vector<UniValue> ret` (memory filled by `ListTransactions`)
* Pass iterators to `push_backV` instead of creating a full copy
* Move memory into `UniValue result` instead of copying it
ACKs for top commit:
shaavan:
Code Review ACK fa8a1c0696
Tree-SHA512: 1c3ca40fc8497134a4141195160e4aa9fe72b3c00c5998c972b58ad0eb498ebea05013f9105bb80e7264c9db1d0e7a2032396a8a4af1f326d831fbee20f32ea3
Overloading sleep_for is not needed, as
* seconds and minutes can be converted to milliseconds by the compiler,
not needing a duration_cast
* std::condition_variable::wait_for will convert milliseconds to the
duration type of the underlying clock
So simply expose the clock.
07f2c25d04 refactor: add most of src/util to iwyu (fanquake)
Pull request description:
These files change infrequently, and not much header shuffling is required.
We don't add everything in src/util/ yet, because IWYU makes some
dubious suggestions, which I'm going to follow up with upstream.
Soon we'll swap `src/util/xyz.cpp` for just `src/util/`.
ACKs for top commit:
hebasto:
ACK 07f2c25d04, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 07d76435c2bff1a62c4967eb0efaafe619cc3bbaf4166741d8520927b24336c01aee59822f8082ee2a01e15046a0f5d506b4b23a6e40ceb750f3226ed8167847
2ef5294a5b rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff test: add getblockfrompeer coverage of invalid inputs (Jon Atack)
Pull request description:
The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
Fix all issues.
ACKs for top commit:
brunoerg:
ACK 2ef5294a5b
Tree-SHA512: 454782cf6a44fd0e05483bb152153667ef5c8021358385ddcf89724fbbbd35e187362bdff757e00c99319527bc4c0b20c7187f67241d4585d767a29787142f25
c6c35db057 wallet: change `ScanForWalletTransactions` to use `Ticks()` (w0xlt)
Pull request description:
This PR changes `ScanForWalletTransactions()` to use the `Ticks(Dur2 d)` function (introduced in #25456).
ACKs for top commit:
MarcoFalke:
cr ACK c6c35db057
Tree-SHA512: 864e136b470baf22293dc03ae3400bbb34955389a1efc83862f006cfac84da9128c3a201ef051606c06f782a1fde84129261dd4b417cbfff854d5c359a92703e
a9790ba95f [test] persist prioritisation of transactions not in mempool (glozow)
Pull request description:
We persist tx prioritisation/fee deltas in mempool.dat (see `DumpMempool`). It seems we have test coverage for persistence of modified fees of mempool entries (see `vinfo` loop), but not for the prioritisation of transactions not in mempool (see `mapDeltas`).
Relevant: https://github.com/bitcoin/bitcoin/pull/25487#discussion_r917490221
ACKs for top commit:
darosior:
utACK a9790ba95f
w0xlt:
ACK a9790ba95f
Tree-SHA512: 3f2769a917041f12414584f69b2239eef57586f9975869e826f356633fcaf893fde15500619b302e7663de14f3661c6cba22c7524cab5286e715e2c105726521
fa4be8e7c3 move-only: InitializeNode to handshake helper (MacroFake)
fa7098947c move-only: Version handshake to libtest_util (MacroFake)
Pull request description:
The version handshake after setting up a peer is an integral part of (unit) testing net processing logic.
Thus, make the helper accessible in libtest_util.
Also, remove the peerman argument from `FillNode`, as it must be equal to connman's peerman, which can then be used instead.
ACKs for top commit:
dergoegge:
ACK fa4be8e7c3
Tree-SHA512: 8296399dc2c29196bd56584c9b61f1c5a088f96dd3438b07b84e1acf525d867f1e37fdfdeede8a831add25848cda0c221ce3fb873e5ae5ca805a1765aa08eb12
111ea3ab71 wallet: refactor GetNewDestination, use BResult (furszy)
22351725bc send: refactor CreateTransaction flow to return a BResult<CTransactionRef> (furszy)
198fcca162 wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' (furszy)
7a45c33d1f Introduce generic 'Result' class (furszy)
Pull request description:
Based on a common function signature pattern that we have all around the sources:
```cpp
bool doSomething(arg1, arg2, arg3, arg4, &result_obj, &error_string) {
// do something...
if (error) {
error_string = "something bad happened";
return false;
}
result = goodResult;
return true;
}
```
Introduced a generic class `BResult` that encapsulate the function boolean result, the result object (in case of having it) and, in case of failure, the string error reason.
Obtaining in this way cleaner function signatures and removing boilerplate code:
```cpp
BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
// do something...
if (error) return "something bad happened";
return goodResult;
}
```
Same cleanup applies equally to the function callers' side as well. There is no longer need to add the error string and the result object declarations before calling the function:
Before:
```cpp
Obj result_obj;
std::string error_string;
if (!doSomething(arg1, arg2, arg3, arg4, result_obj, error_string)) {
LogPrintf("Error: %s", error_string);
}
return result_obj;
```
Now:
```cpp
BResult<Obj> op_res = doSomething(arg1, arg2, arg3, arg4);
if (!op_res) {
LogPrintf("Error: %s", op_res.GetError());
}
return op_res.GetObjResult();
```
### Initial Implementation:
Have connected this new concept to two different flows for now:
1) The `CreateTransaction` flow. --> 7ba2b87c
2) The `GetNewDestination` flow. --> bcee0912
Happy note: even when introduced a new class into the sources, the amount of lines removed is almost equal to added ones :).
Extra note: this work is an extended version (and a decoupling) of the work that is inside #24845 (which does not contain the `GetNewDestination` changes nor the inclusion of the `FeeCalculation` field inside `CreatedTransactionResult`).
ACKs for top commit:
achow101:
ACK 111ea3ab71
w0xlt:
reACK 111ea3ab71
theStack:
re-ACK 111ea3ab71
MarcoFalke:
review ACK 111ea3ab71🎏
Tree-SHA512: 6d84d901a4cb923727067f25ff64542a40edd1ea84fdeac092312ac684c34e3688a52ac5eb012717d2b73f4cb742b9d78e458eb0e9cb9d6d72a916395be91f69
d5c141f221 qt: apply translator comments to reset options confirmation dialog (Jarol Rodriguez)
Pull request description:
This is a followup to #617. Because the strings were being concatenated, we can not apply translator comments to all of the revelant strings. This can be tested by applying the following diff to current master and running `make translate`; then check the resulting diff:
```diff
diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index 462b923d6..3cf165004 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsdialog.cpp
@@ -286,9 +286,17 @@ void OptionsDialog::on_resetButton_clicked()
{
if (model) {
// confirmation dialog
+ //: Window title text of pop-up window shown when the user has chosen to reset options.
QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
+ /*: Text explaining that the settings the user changed will not come
+ into effect until the client is restarted. */
tr("Client restart required to activate changes.") + "<br><br>" +
+ /*: Text explaining to the user that the client's current settings
+ will be backed up at a specific location. %1 is a stand-in
+ argument for the backup location's path. */
tr("Current settings will be backed up at \"%1\".").arg(m_client_model->dataDir()) + "<br><br>" +
+ /*: Text asking the user to confirm if they would like to proceed
+ with a client shutdown. */
tr("Client will be shut down. Do you want to proceed?"),
QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);
```
To apply the above translator comments, what we want to do instead is have a variable in which the translatable strings are appended to using the [QString append function](https://doc.qt.io/qt-5/qstring.html#append).
When you run `make translate` with this PR, you will see the translator comments properly applied, as shown below:
``` diff
diff --git a/src/qt/locale/bitcoin_en.ts b/src/qt/locale/bitcoin_en.ts
index 35d820187..9e5158b3e 100644
--- a/src/qt/locale/bitcoin_en.ts
+++ b/src/qt/locale/bitcoin_en.ts
@@ -1942,28 +1942,37 @@ Signing is only possible with addresses of the type 'legacy'.</source>
<translation>default</translation>
</message>
<message>
- <location line="+81"/>
+ <location line="+86"/>
<source>none</source>
<translation type="unfinished"></translation>
</message>
<message>
- <location line="+97"/>
+ <location line="+107"/>
<source>Confirm options reset</source>
+ <extracomment>Window title text of pop-up window shown when the user has chosen to reset options.</extracomment>
<translation>Confirm options reset</translation>
</message>
<message>
- <location line="+1"/>
- <location line="+70"/>
+ <location line="-9"/>
+ <location line="+79"/>
<source>Client restart required to activate changes.</source>
+ <extracomment>Text explaining that the settings changed will not come into effect until the client is restarted.</extracomment>
+ <translation type="unfinished"></translation>
+ </message>
+ <message>
+ <location line="-75"/>
+ <source>Current settings will be backed up at "%1".</source>
+ <extracomment>Text explaining to the user that the client's current settings will be backed up at a specific location. %1 is a stand-in argument for the backup location's path.</extracomment>
<translation type="unfinished"></translation>
</message>
<message>
- <location line="-70"/>
+ <location line="+3"/>
<source>Client will be shut down. Do you want to proceed?</source>
+ <extracomment>Text asking the user to confirm if they would like to proceed with a client shutdown.</extracomment>
<translation type="unfinished"></translation>
</message>
<message>
- <location line="+18"/>
+ <location line="+20"/>
<source>Configuration options</source>
<extracomment>Window title text of pop-up box that allows opening up of configuration file.</extracomment>
<translation type="unfinished"></translation>
```
No difference in rendering between master and PR
| master | PR |
| ------- | --- |
<img width="532" alt="Screen Shot 2022-06-29 at 11 39 17 PM" src="https://user-images.githubusercontent.com/23396902/176588495-9d3761b6-9d96-489a-bbe5-a8907f7d5f99.png"> | <img width="532" alt="Screen Shot 2022-06-29 at 11 39 51 PM" src="https://user-images.githubusercontent.com/23396902/176588513-92e29564-b74a-46f5-a5dd-469c4ee953f7.png"> |
ACKs for top commit:
shaavan:
ACK d5c141f221
furszy:
Tested ACK d5c141f2, no functional changes.
w0xlt:
tACK d5c141f221
Tree-SHA512: 6175a096c6f99edb3041cc2429e1ea0670a10cd2cab0364f664a56c7dee1aa8631d52c0a36edb5d571f6ef934e947d45017e446cea7dddae044085c39c8835ef
dc02edcba1 doc: update the URLs to thread functions in developer-notes (Vasil Dimov)
c5cc3f140c doc: list the I2P accept thread in developer-notes (Vasil Dimov)
Pull request description:
Document `i2paccept` in `doc/developer-notes.md` and fix broken URLs to doxygen.bitcoincore.org.
ACKs for top commit:
theStack:
re-ACK dc02edcba1
Tree-SHA512: 7d396885dd2e8fda2b050aaa25a82b4217ced6a5aa3478339fb892d5392d2b8b6b5997f8bb9acaab7867c0c5bf58bd0b720ef36b335b1e7eb617b8fc205915b0
76a84c0a6c test: speedup wallet_coinbase_category.py (furszy)
Pull request description:
No need to create a chain (200 extra blocks), nor use the cache, for it.
Top commit has no ACKs.
Tree-SHA512: beec64ba6c580d475e19700371ae155f4f3d74325879802da83d02f4153a752d5829b8a4ed77e0c893e79cbc26f66389eed90ac2e8b03a59f6c630ee9333355c
230a2f4cc3 wallet test: Add unit test for wallet scan save_progress option (Ryan Ofsky)
a89ddfbe22 wallet: Save wallet scan progress (w0xlt)
Pull request description:
Currently, the wallet scan progress is not saved.
If it is interrupted, it will be necessary to start from scratch on the next load.
This PR changes this and the progress is saved right after checking a block.
Close https://github.com/bitcoin/bitcoin/issues/25010
ACKs for top commit:
furszy:
re-ACK 230a2f4
achow101:
ACK 230a2f4cc3
ryanofsky:
Code review ACK 230a2f4cc3. Only change since last review is tweaking whitespace and adding log print
Tree-SHA512: 1a9dec207ed22b3443fb06a4daf967637bc02bcaf71c070b7dc33605d0cab959551e4014c9e92293a63f54c5cbcc98bb9f8844a8c60bc32a1482b1c4130fab32
If we are given a PSBT that is missing one or more input UTXOs, our
PrecomputedTransactionData will be incorrect and missing information
that it should otherwise have, and therefore we may not produce a
signature when we should. To avoid this problem, we can do the
precomputation after we have set the UTXOs the wallet is able to set for
the PSBT.
Also adds a test for this behavior.
98ea43d5e9 test: add tests for negative waste during coin selection (ishaanam)
Pull request description:
#25495 mentions that waste can be negative when the current feerate is less than the long term feerate. There are currently no waste tests for negative waste, so this PR adds two of them.
ACKs for top commit:
achow101:
ACK 98ea43d5e9
glozow:
light code review ACK 98ea43d5e9, good to have tests for negative waste
Tree-SHA512: d194d370f1257975959d3c601fea9f82c30c1aabc3e8bedc997c62659283fe681cc527e59df1a0187b3c91e8067c60374dd5ce0237561bd882edafe6a575a9b9
0ee43d13e9 test: refactor rpc_signrawtransaction.py (Ayush Sharma)
Pull request description:
`rpc_signrawtransaction.py` currently tests the `signrawtransactionwithkey` and `signrawtransactionwithwallet` RPCs.
This PR splits `rpc_signrawtransaction.py` into
1. `rpc_signrawtransactionwithkey.py`: the tests for `signrawtransactionwithkey` are moved here and this test can now be run with the wallet disabled.
2. `wallet_signrawtransactionwithwallet.py`: wallet only tests for `signrawtransactionwithwallet.py`
ACKs for top commit:
aureleoules:
tACK 0ee43d13e9.
Tree-SHA512: c7bd2ea746345c978eae231a781fc52953b9d277eb9f8abb9c3270fe1d9f678f23f3784377d7303a2aa23d7ad90097245e517d386b27b4e0016585dfddcb9d49
6cbe65c5d7 test: refactor: pass absolute fee in `create_lots_of_big_transactions` helper (Sebastian Falbesoner)
Pull request description:
Recently merged PR #25522 (commit 2222842ae7) enabled specifying an absolute fee for MiniWallet's `create_self_transfer` method. We can use that in the `create_lots_of_big_transactions` helper to avoid deducting the fee manually (with prior conversion from BTC to Satoshis). This helper is used (directly or indirectly) in the tests `feature_maxuploadtarget.py`, `mempool_limit.py`, `mining_prioritisetransaction.py`.
ACKs for top commit:
MarcoFalke:
cr ACK 6cbe65c5d7
Tree-SHA512: 63d66939ae36722a2dc787cbd8f1f995de6232139c2169a3d25525f43c7aaacf646d86b4095a8078f26db18e916778c8097acb19ef17ab0f58382b8bb718d60b
d54c5c8b1b wallet: use CCoinControl to estimate signature size (S3RK)
a94659c84e wallet: replace GetTxSpendSize with CalculateMaximumSignedInputSize (S3RK)
Pull request description:
Currently `DummySignTx` and `DummySignInput` use different ways to determine signature size.
This PR unifies the way wallet estimates signature size for various inputs.
Instead of passing boolean flags from calling code the `use_max_sig` is now calculated at the place of signature creation using information available in `CCoinControl`
ACKs for top commit:
achow101:
ACK d54c5c8b1b
theStack:
Code-review ACK d54c5c8b1b
Tree-SHA512: e790903ad4683067070aa7dbf7434a1bd142282a5bc425112e64d88d27559f1a2cd60c68d6022feaf6b845237035cb18ece10f6243d719ba28173b69bd99110a
Useful to encapsulate the function result object (in case of having it) or, in case of failure, the failure reason.
This let us clean lot of boilerplate code, as now instead of returning a boolean and having to add a ref arg for the
return object and another ref for the error string. We can simply return a 'BResult<Obj>'.
Example of what we currently have:
```
bool doSomething(arg1, arg2, arg3, arg4, &result, &error_string) {
do something...
if (error) {
error_string = "something bad happened";
return false;
}
result = goodResult;
return true;
}
```
Example of what we will get with this commit:
```
BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
do something...
if (error) return {"something happened"};
// good
return {goodResult};
}
```
This allows a similar boilerplate cleanup on the function callers side as well. They don't have to add the extra
pre-function-call error string and result object declarations to pass the references to the function.
d69045e291 test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy)
324f00a642 refactor: 'ListReceived' use optional for filtered address (furszy)
b459fc122f refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy)
fa9f2ab8fd refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy)
83e42c4b94 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy)
2b48642499 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy)
032842ae41 wallet: implement ForEachAddrBookEntry method (furszy)
09649bc95d refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy)
192eb1e61c refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy)
Pull request description:
### Context
The wallet's `m_address_book` field is being accessed directly from several places across the sources.
### Problem
Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own.
### Solution
Encapsulate `m_address_book` access inside the wallet.
-------------------------------------------------------
Extra Note:
This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR).
ACKs for top commit:
achow101:
ACK d69045e291
theStack:
ACK d69045e291✅
w0xlt:
ACK d69045e291
Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
d3e9a1c71b doc: update for NetBSD 9.2, add GUI Build Instructions (Jarol Rodriguez)
Pull request description:
**For reviewer:** as I suppose few have a NetBSD system available, I wrote a [guide](https://gist.github.com/jarolrod/385dc063bb02c90aea0cbe8a147fc418#file-netbsd-vm-setup-guide-md) to setup a VM for testing purposes.
This attempts to update the NetBSD docs so one can successfully build on the latest release. It also adds instructions to build the GUI.
Additionally, it includes a note and an example on how one could update the gcc version bundled with NetBSD 9.2 and prior to be able to actually compile. This note can be updated with the release of NetBSD 10, as it will package an acceptable gcc version.
Master: [render](https://github.com/bitcoin/bitcoin/blob/master/doc/build-netbsd.md)
PR: [render](d3e9a1c71b/doc/build-netbsd.md)
Related to #20610, but reworked.
ACKs for top commit:
aureleoules:
ACK d3e9a1c71b.
fanquake:
ACK d3e9a1c71b
Tree-SHA512: fc3c12689cee886f26782c1d57f3b794ceaedc965a571dd06cfc4a57f90393842ad2124e6dba55a12ac9de9bf63d8e3eb4aa541768f2aa8603248175ce7d1c08
These files change infrequently, and not much header shuffling is required.
We don't add everything in src/util/ yet, because IWYU makes some
dubious suggestions, which I'm going to follow up with upstream.
