Instead of using ismine to check whether an address can be spent by us, make the witness version of the script or address first and then use ProduceSignature with the DummySignatureCreator to check if we can solve for the script.
This is to fix cases where we don't have all of the private keys (for something like a multisig address) but have the redeemscript so we can witnessify it.
Concept ACK, but you'll need to adapt the tests (there is a test in segwit.py which specifically tests for this behaviour).
Travis failed on an unrelated call to encryptwallet() in wallet-dump.py: https://travis-ci.org/bitcoin/bitcoin/jobs/252653271 . I've restarted the job.
(encryptwallet() causes the node to shutdown, so I guess there's a race between the node shutting down and responding to the RPC. That's unrelated to this PR and is something we should think about fixing independently)
ACK.
utACK db5cd10bbe1688a5e05e558d1323f8cab923567c, but needs squash before merge.
458 | @@ -459,13 +459,15 @@ def run_test(self): 459 | self.mine_and_test_listunspent(unsolvable_after_importaddress, 1) 460 | self.mine_and_test_listunspent(unseen_anytime, 0) 461 | 462 | - # addwitnessaddress should refuse to return a witness address if an uncompressed key is used or the address is 463 | - # not in the wallet 464 | + # addwitnessaddress should refuse to return a witness address if an uncompressed key is used 465 | # note that no witness address should be returned by unsolvable addresses 466 | # the multisig_without_privkey_address will fail because its keys were not added with importpubkey
This comment should be updated/removed.
Seems like this is the right general direction to me (seems vastly simpler than what happens in IsMine()), though I have two concerns that I need to think more about before I can ACK:
The protection against accidentally allowing an uncompressed pubkey in a segwit script via addwitnessaddress has now moved from the logic in IsMine() to the VerifyScript() call at the end of ProduceSignature. First, I think that's generally surprising and I could imagine someone not understanding that it's important that we call VerifyScript at the end of ProduceSignature. But second, I think it's unexpected that the script flags passed to VerifyScript in ProduceSignature (which are just our regular policy script flags) are what we're relying on for this wallet behavior. The policy script flags are not something that generally affects our wallet, so I think it'd be unexpected if someone tweaked theirs and got different wallet behavior as a result.
Along the lines of the above, it seems like we could use some tests to ensure that IsMine() interacts with ProduceSignature() as expected. In the above example of someone tweaking their policy script flags (eg to turn off SCRIPT_VERIFY_WITNESS_PUBKEYTYPE) then I think you could get into a situation where you could add an address to your wallet that fails IsMine(). This is pretty edge-case, but I want to spend more time thinking about whether other kinds of inconsistencies could exist between IsMine() and this ProduceSignature(...) method.
Instead of using ismine to check whether an address can be spent by us,
make the witness version of the script or address first and then use
ProduceSignature with the DummySignatureCreator to check if we can
solve for the script.
Also fixes test cases to reflect this change.
utACK e222dc2