743a84a5f6 fix gettxout help text (Marnix)
Pull request description:
replaces #25578
Add help text to asm & hex (like everywhere else).
I've also changed two `RPCResult::Type::STR` to `RPCResult::Type::STR_HEX`
Top commit has no ACKs.
Tree-SHA512: 4109d6abddf71b24899f3252545248bb0c7cc366eb994d30927eb300d0b939a14b8140bac4a4c2bd45098a406666dbe1feb10da8dec923777bb8ed26784dfd54
2c3ee4c347 gui: Load Base64 PSBT string from file (Andrew Chow)
Pull request description:
Some .psbt files may have the PSBT as a base64 string instead of in binary. We should be able to load those files.
ACKs for top commit:
jarolrod:
tACK 2c3ee4c347
shaavan:
ACK 2c3ee4c347
Tree-SHA512: 352b0611693c8989ea7d1b8d494ea58c69dc15cf81b8d62271541832e74b0a0399cb6ed4e686ab7c741cb4e5374527e054a9ecfe7355bc6f77d8fdd13569ab76
Also:
1. Have CChainState::LoadMempool and ::ThreadImport take in paths and
pass it through untouched to LoadMempool.
2. Make LoadMempool exit early if the load_path is empty.
3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass
in an empty path if mempool persistence is disabled.
Not only does this increase coverage, it is also more correct in that
when ::LoadMempool is called with a mempool and chainstate, it calls
AcceptToMemoryPool with just the chainstate.
AcceptToMemoryPool will then act on the chainstate's mempool via
CChainState::GetMempool, which may be different from the mempool
originally passed to ::LoadMempool. (In this fuzz test's case, it
definitely is different)
Also, move DummyChainstate to its own file since it's now used by the
validation_load_mempool fuzz test to replace CChainState's m_mempool.
Watchonly wallets do not have any private keys to encrypt. It does not
make sense to encrypt such wallets, so disable the option to encrypt
them.
This avoids an assertion that can be hit when encrypting watchonly descriptor
wallets.
m_is_loaded/IsLoaded() doesn't actually indicate whether or not the
mempool was successfully, loaded, but rather if a load has been
attempted and did not result in a catastrophic ShutdownRequested.
-BEGIN VERIFY SCRIPT-
find_regex="\bm_is_loaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@m_load_tried@g"
find_regex="\bIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@GetLoadTried@g"
find_regex="\bSetIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@SetLoadTried@g"
-END VERIFY SCRIPT-
fa277cd55d univalue: Throw exception on invalid pushes over silent ignore (MacroFake)
ccccc17b91 refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL (MacroFake)
Pull request description:
The return value of the `push*` helpers is never used, but important to determine if the operation was successful. One way to fix this would be to add the "nodiscard" attribute. However, this would make the code (and this diff) overly verbose for no reason.
So fix it by removing the never used return value. Also, fail verbosely in case of a programming mistake.
ACKs for top commit:
furszy:
code ACK fa277cd5
Tree-SHA512: ef212a5bf5ae6bbad20acc4dafa3715521e81544185988d1eab724f440e4864a27e686aff51d5bc51b3017892c2eb8e577bcb8f37e8ddbaa0d8833bb622f2f9c
Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions
in node/mempool_persist_args.{h,cpp} which are used by non-kernel
DumpMempool callers to determine whether or not to automatically dump
the mempool and where to dump it to.
ffc79b8e49 qa: functional test Miniscript watchonly support (Antoine Poinsot)
bfb036756a Miniscript support in output descriptors (Antoine Poinsot)
4a082887be qa: better error reporting on descriptor parsing error (Antoine Poinsot)
d25d58bf5f miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot)
c38c7c5817 miniscript: don't check for top level validity at parsing time (Antoine Poinsot)
Pull request description:
This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of #24147 for a description of Miniscript and a rationale of having it in Bitcoin Core.
On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support.
A minified corpus of Miniscript Descriptors for the `descriptor_parse` fuzz target is available at https://github.com/bitcoin-core/qa-assets/pull/92.
The Miniscript descriptors used in the unit tests here and in #24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript.
This PR contains code and insights from Pieter Wuille.
ACKs for top commit:
Sjors:
re-utACK ffc79b8e49
achow101:
ACK ffc79b8e49
w0xlt:
reACK ffc79b8e49
Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9
Prior to this commit, the peer was connected, and then the services and
connectivity fields in the CNode object were manually set. Instead, send
p2p `version` and `verack` messages, and have net_processing's internal
logic set the state of the node.
This ensures that the node's internal state is consistent with how it
would be set in the live code.
Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE`
which was not a problem since `CNode::fClient` and
`CNode::m_limited_node` are default initialised to false. Now that we
are doing the actual version handshake, the values of `fClient` and
`m_limited_node` are set during the handshake and cause the test to fail
if we do not set `dummyNode1.nServices` to a reasonable value
(NODE_NETWORK | NODE_WITNESS).
Miniscript descriptors are defined under P2WSH context (either `wsh()`
or `sh(wsh())`).
Only sane Miniscripts are accepted, as insane ones (although valid by
type) can have surprising behaviour with regard to malleability
guarantees and resources limitations.
As Miniscript descriptors are longer and more complex than "legacy"
descriptors, care was taken in error reporting to help a user determine
for what reason a provided Miniscript is insane.
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
This is helpful for finer grained descriptor parsing error: when there
are multiple errors to report in a Miniscript descriptor start with the
"smallest" fragments: the ones closer to be a leaf.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
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
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
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
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
