wallet: fix change detection of imported internal descriptors #20266
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Is this for 0.21? |
Yes, this is a bug fix. |
meshcollider
left a comment
There was a problem hiding this comment.
utACK bd93fc9
Good catch
|
@S3RK you might be interested in reviewing? |
|
|
||
| CTxDestination dest; | ||
| if (ExtractDestination(script_pub_keys.at(0), dest)) { | ||
| if (!internal && ExtractDestination(script_pub_keys.at(0), dest)) { |
There was a problem hiding this comment.
Is it expected that we will set an empty label even when a label is not specified in request at all?
nit: I think it's more clear to make all the checks in the parent if (for range, internal and for label emptiness). This also might help to avoid unnecessary log message above.
There was a problem hiding this comment.
Is it expected that we will set an empty label even when a label is not specified in request at all?
Yes.
There was a problem hiding this comment.
On second thought, nevermind. Reverted back to previous.
Moving it up will make the boolean logic at the outer if a little more confusing, and I think it's fine (perhaps even good) to be checking that scriptPubKeys were being generated for non-ranged things.
There was a problem hiding this comment.
On second thought, nevermind. Reverted back to previous.
Moving it up will make the boolean logic at the outer if a little more confusing, and I think it's fine (perhaps even good) to be checking that scriptPubKeys were being generated for non-ranged things.
Though the check becomes redundant after #20153 I think I maybe even should drop it in my PR
2b35435 to
bd93fc9
Compare
|
Code review ACK bd93fc9 |
Summary: Import internal descriptors were having address book entries added which meant they would be detected as non-change. Fix this and add a test for it. This is a backport of [[bitcoin/bitcoin#20266 | core#20266]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10719
Import internal descriptors were having address book entries added which meant they would be detected as non-change. Fix this and add a test for it.