rpc: Exclude descriptor when address is excluded #24636
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Completely reworked the pull |
|
ACK faf37c2 |
jonatack
reviewed
Mar 24, 2022
There was a problem hiding this comment.
ACK faf37c2
This change adds a "desc" field that was missing from the "witness_utxo" decodepsbt inputs help and IIUC removes undocumented "desc" descriptor fields from the decodepsbt inputs and outputs "redeem_script" and "witness_script" result objects.
Code review:
ScriptPubKeyToUniv() is declared with include_address = true by default
src/core_io.h:56:void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address = true);and is invoked once in decodepsbt in the witness inputs code
// inputs
CAmount total_in = 0;
bool have_all_utxos = true;
UniValue inputs(UniValue::VARR);
for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
const PSBTInput& input = psbtx.inputs[i];
UniValue in(UniValue::VOBJ);
// UTXOs
bool have_a_utxo = false;
CTxOut txout;
if (!input.witness_utxo.IsNull()) {
txout = input.witness_utxo;
UniValue o(UniValue::VOBJ);
ScriptPubKeyToUniv(txout.scriptPubKey, o, /* include_hex */ true);
UniValue out(UniValue::VOBJ);
out.pushKV("amount", ValueFromAmount(txout.nValue));
out.pushKV("scriptPubKey", o);
in.pushKV("witness_utxo", out);
have_a_utxo = true;
}relevant updated decodepsbt help
"inputs" : [ (json array)
{ (json object)
"non_witness_utxo" : { (json object, optional) Decoded network transaction for non-witness UTXOs
...
},
"witness_utxo" : { (json object, optional) Transaction output for witness UTXOs
"amount" : n, (numeric) The value in BTC
"scriptPubKey" : { (json object)
"asm" : "str", (string) The asm
"desc" : "str", (string) Inferred descriptor for the output
"hex" : "hex", (string) The hex
"type" : "str", (string) The type, eg 'pubkeyhash'
"address" : "str" (string, optional) The Bitcoin address (only if a well-defined address exists)
}
OTOH ScriptToUniv() is the only caller to invoke ScriptPubKeyToUniv() with include_address = false, and outside of a fuzz test is only called in decodepsbt inputs and outputs "redeem_script" and "witness_script" result code and so with this pull the descriptor will be excluded
void ScriptToUniv(const CScript& script, UniValue& out)
{
ScriptPubKeyToUniv(script, out, /* include_hex */ true, /* include_address */ false);
}|
Backported to v23.0 in #24512. |
jonatack
pushed a commit
to jonatack/bitcoin
that referenced
this pull request
Mar 28, 2022
Github-Pull: bitcoin#24636 Rebased-From: faf37c2
Merged
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Mar 31, 2022
Github-Pull: bitcoin#24636 Rebased-From: faf37c2
jonatack
pushed a commit
to jonatack/bitcoin
that referenced
this pull request
Mar 31, 2022
Github-Pull: bitcoin#24636 Rebased-From: faf37c2
fanquake
added a commit
that referenced
this pull request
Mar 31, 2022
174af33 util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem) (Hennadii Stepanov) ded10fe build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov) 26c2f23 build: Fix Boost.Process detection on macOS arm64 (Hennadii Stepanov) 85f85c7 util: add linkat to syscall sandbox (AllowFileSystem) (fanquake) eaa0419 contrib: fix signet miner (sighash mismatch) (Sebastian Falbesoner) 235b042 rpc: Exclude descriptor when address is excluded (MarcoFalke) b05a59b ci: Temporarily use clang-13 to work around clang-14 TSan bug (MarcoFalke) 65b9667 doc, init: add links to doc/cjdns.md (Jon Atack) 7a553d4 doc: update i2p.md with cjdns, improve local addresses section (Jon Atack) 4148396 doc: update tor.md with cjdns and getnodeaddresses, fix tor grep, (Jon Atack) 4690e8a doc: create initial doc/cjdns.md for cjdns how-to documentation (Jon Atack) 5d24f61 Clarify in -maxtimeadjustment that only outbound peers influence time data (Jon Atack) b1646f1 test: set segwit height back to 0 on regtest (Martin Zumsande) ef6a37b rpc: rename getdeploymentinfo status-next to status_next (Jon Atack) 2a6fcf9 init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack) Pull request description: Backport the following to 23.x: - #24468 - #24528 - #24527 - #24609 - #24555 - #24663 - #24572 - #24636 - #24553 - #24659 - #24521 - #24523 - #24690 - #24710 Possibly also: - #24579 - #24691 ACKs for top commit: laanwj: List-of-commits ACK 174af33, I think we should merge this and move forward with rc3.. hebasto: ACK 174af33 Tree-SHA512: 5a493e1652b780b527767d6ca9e67012abd2fa5573496e85e0d8aa4bed3eb332bfcd72610b8dfb954ff274d42450623233c96c479de2085b9c8344ba5abf1935
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 2, 2022
faf37c2 rpc: Exclude descriptor when address is excluded (MarcoFalke) Pull request description: I don't think output descriptors should be used to describe redeem scripts and witness scripts. Fix this by excluding them when it doesn't make sense. This should only affect the `decodepsbt` RPC. Found by bitcoin#23083 ACKs for top commit: achow101: ACK faf37c2 jonatack: ACK faf37c2 Tree-SHA512: ebd581ad639e70080e26028723fed287caa3fa4d7b836936645020d6cd9b7586585d7113b043442c444a9dc90c23b93efd7f8b8a7d6cf5db1e42137b67c497c3
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I don't think output descriptors should be used to describe redeem scripts and witness scripts.
Fix this by excluding them when it doesn't make sense.
This should only affect the
decodepsbtRPC.Found by #23083