-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add address field (rpc client & server) #1969
Conversation
Pull Request Test Coverage Report for Build 5193275574
💛 - Coveralls |
PR looks fine! I wanted to open a similar PR because we now receive the address field from the node and don't have it in the result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to break backward compatibility with Bitcoin Core 0.21 or earlier. So I think the proper fix would be to accept both fields and then parse the one that's actually being set by the chain backend.
faae231
to
c6c7794
Compare
Okay @guggero I've left the deprecated fields in. The commit to remove them exists in lindlof@faae231 |
rpcserver.go
Outdated
@@ -732,6 +732,9 @@ func createVoutList(mtx *wire.MsgTx, chainParams *chaincfg.Params, filterAddrMap | |||
var vout btcjson.Vout | |||
vout.N = uint32(i) | |||
vout.Value = btcutil.Amount(v.Value).ToBTC() | |||
if len(encodedAddrs) == 1 && reqSigs == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure the reqSigs
is always set to 1 on old versions? Can't we just omit that condition? And I guess a comment above about why we do this (backward compatibility) would be useful too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the condition because if more than 1 signatures are required then we shouldn't consider the address to be the only address. This could result in terrible bugs, such as the consumer believing they have received the Bitcoin when it's actually in a multisig.
I'll double-check and can add a comment and make it also accept 0 as long as an address is available. The reqSigs
should be correct but I agree it makes sense for this to be robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment and changed the reqSigs
check to also accept 0. ExtractPkScriptAddrs
simply returns the reqSigs
as 1 whenever an address is returned, except for multisig which has more logic for reqSigs
and where checking the value of reqSigs
is more interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also checked that ExtractPkScriptAddrs
returns a single address and reqSigs == 1
for P2PKH, P2WPKH, P2SH, P2WSH, and P2TR.
The change would only be meaningful to non standard multi-sig.
c6c7794
to
e4c88c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
I guess another review from someone who has more experience with the code base than myself would be great. Any volunteers, @Roasbeef, @kcalvinalvin, @chappjc? |
ACK fcf291947d51896664e848526482983a5cbb72db26fa1d78649c3d051cf602fc Also tested by calling |
Bitcoin Core has added
address
field and removedaddresses
andreqSigs
rpc response fields. The fields were deprecated on version 22 and they've been removed from version 23. For instance, the following methods are affected:This PR adds the new
address
and deprecates the removed fields.Closes #1874