wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag #20191
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. |
|
ACK 07d2fff |
|
concept ACK |
07d2fff to
c9a4e0d
Compare
c9a4e0d to
7e329e2
Compare
There was a problem hiding this comment.
utACK
Idea for follow-up:
Extract the desc_str creation from DescriptorScriptPubKeyMan::SetupDescriptorGeneration, then I think the logic/flow between ExternalSignerScriptPubKeyMan and DescriptorScriptPubKeyMan becomes a lot closer, could reduce amount of required duplicate code.
It would also mean that DescriptorSPKM never have to know anything about internal-ness, just like External ones.
|
ACK c94e3da |
Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating such information could lead to inconsistencies and unexpected behaviour.
|
reACK 1811810 |
|
reACK 1811810 |
|
Looks like there are now some CI failures for the |
… agnostic of internal flag 1811810 refactor: remove m_internal from DescriptorSPKman (S3RK) Pull request description: Rationale: improve consistency between `CWallet` and `DescriptorScriptPubKeyMan`; simplify `ScriptPubKeyMan` interface. Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size). ACKs for top commit: instagibbs: reACK bitcoin@1811810 achow101: reACK 1811810 Tree-SHA512: d5613b7f6795b290bfa0fd8cb0536de1714d0cf72cba402266bd06d550758ebad690b54fc0a336a1c7414b5814aa4a37c90a6ae89926474a97d30956d7e034ff
This broke functional tests; had to forward-port #22379 (which will be merged in a couple PRs) to fix it.
Rationale: improve consistency between
CWalletandDescriptorScriptPubKeyMan; simplifyScriptPubKeyManinterface.Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size).