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. |
guggero
left a comment
There was a problem hiding this comment.
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
| 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.
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.
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.
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.
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
|
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
addressfield and removedaddressesandreqSigsrpc 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
addressand deprecates the removed fields.Closes #1874