wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag#20191
wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag#20191fanquake merged 1 commit intobitcoin:masterfrom
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 07d2fffea9ea3589060632fdfdbf44f9d31caab6 |
|
concept ACK |
S3RK
force-pushed
the
remove_m_internal
branch
from
07d2fff to
c9a4e0d
Compare
achow101
left a comment
achow101
left a comment
There was a problem hiding this comment.
re-ACK c9a4e0d8e2846e7eb03e8d5fd3ee18dba6f3c4a1
Only change since last review is a comment.
meshcollider
left a comment
meshcollider
left a comment
There was a problem hiding this comment.
utACK c9a4e0d8e2846e7eb03e8d5fd3ee18dba6f3c4a1
S3RK
force-pushed
the
remove_m_internal
branch
from
c9a4e0d to
7e329e2
Compare
achow101
left a comment
achow101
left a comment
There was a problem hiding this comment.
ACK 7e329e281042051fd85acd2f9dd8f893cbefcb8d
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 c94e3da57e85812ad391e4f7173321781419b9c0 |
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.
7 participants
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).