Check descriptors returned by external signers #23628
Conversation
sstone
force-pushed
the
external-signer-catch-invalid-scriptors
branch
from
c098529 to
f9574a4
Compare
|
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. |
sstone
force-pushed
the
external-signer-catch-invalid-scriptors
branch
2 times, most recently
from
be42c78 to
0221b2c
Compare
test/functional/wallet_signer.py
Outdated
|
|
||
| def mock_signer_path(self): | ||
| path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py') | ||
| path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'invalid_signer.py' if self.options.invalid else 'signer.py') |
There was a problem hiding this comment.
Adding a new invalid_signer.py is fine with me, though an alternative approach would be to keep one mock, and then use the device fingerprint to decide the behavior.
In that case you would expand the enumerate mock to add a signer with e.g. "fingerprint": "00000003", and then getdescriptors would return an invalid descriptor for 00000003.
There was a problem hiding this comment.
I could not get that to work, it seems that it is not supported yet ? (see https://github.com/bitcoin/bitcoin/blob/master/src/wallet/external_signer_scriptpubkeyman.cpp#L47) so instead I re-start the node and change signer path to point to the invalid signer mock.
There was a problem hiding this comment.
createwallet doesn't let you specify a fingerprint, so this workaround is probably fine for now.
| buffer = sys.stdin.read() | ||
| if buffer and buffer.rstrip() != "": | ||
| sys.argv.extend(buffer.rstrip().split(" ")) |
There was a problem hiding this comment.
python3 -c "import sys; print(type(sys.stdin))" reports that this is a TextIOWrapper (even when I pipe something in such that stdin isn't a tty), so that BufferedIOBase.read will be used, which cannot return None. Why not buffer = sys.stdin.read().rstrip(), then? Then it wouldn't be necessary to strip it twice. And if buffer and buffer.rstrip() != "" could be simply if buffer or maybe if buffer != "" if you prefer to be verbose.
There was a problem hiding this comment.
This was copied from the original https://github.com/bitcoin/bitcoin/blob/master/test/functional/mocks/signer.py code.
sstone
force-pushed
the
external-signer-catch-invalid-scriptors
branch
from
0221b2c to
a5f53c3
Compare
sstone
force-pushed
the
external-signer-catch-invalid-scriptors
branch
from
a5f53c3 to
96846ef
Compare
|
ACK 96846ef7817257326d51c4f05047ca46c3f7f3c7 |
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
tACK 96846ef7817257326d51c4f05047ca46c3f7f3c7
Check that descriptors returned by external signers have been parsed properly when creating a new wallet.
sstone
force-pushed
the
external-signer-catch-invalid-scriptors
branch
from
96846ef to
5493e92
Compare
|
Code review ACK 5493e92 Nice first contribution! Fix is straightforward and well-covered by tests. I did not test this locally. |
|
ACK 5493e92 |
5493e92 Check descriptors returned by external signers (sstone) Pull request description: Check that descriptors returned by external signers have been parsed properly when creating a new wallet. See bitcoin#23627 for context. The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`. I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`. ACKs for top commit: jamesob: Code review ACK bitcoin@5493e92 achow101: ACK 5493e92 Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04
…igners c904665 Check descriptors returned by external signers (sstone) Pull request description: Check that descriptors returned by external signers have been parsed properly when creating a new wallet. See bitcoin/bitcoin#23627 for context. The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`. I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`. ACKs for top commit: jamesob: Code review ACK bitcoin/bitcoin@c904665 achow101: ACK c904665 Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04
8 participants
Check that descriptors returned by external signers have been parsed properly when creating a new wallet.
See #23627 for context.
The problem is that parsing an invalid descriptor will return
nullwhich is not checked for inCWallet::SetupDescriptorScriptPubKeyMans().I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in
CWallet::SetupDescriptorScriptPubKeyMans().