Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
[RPC] Fix addwitnessaddress by replacing ismine with producesignature #10788
Conversation
achow101
changed the title from
Replace ismine with producesignature check in witnessifier to [RPC] Fix addwitnessaddress by replacing ismine with producesignature
Jul 11, 2017
|
Concept ACK, but you'll need to adapt the tests (there is a test in segwit.py which specifically tests for this behaviour). |
laanwj
added the
RPC/REST/ZMQ
label
Jul 11, 2017
|
@sipa I have adapted segwit.py to reflect the new behavior. |
|
Travis failed on an unrelated call to ( |
|
@sdaftuar Would you mind reviewing this? |
|
utACK db5cd10, but needs squash before merge. |
sdaftuar
reviewed
Jul 19, 2017
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.
| @@ -459,13 +459,15 @@ def run_test(self): | ||
| self.mine_and_test_listunspent(unsolvable_after_importaddress, 1) | ||
| self.mine_and_test_listunspent(unseen_anytime, 0) | ||
| - # addwitnessaddress should refuse to return a witness address if an uncompressed key is used or the address is | ||
| - # not in the wallet | ||
| + # addwitnessaddress should refuse to return a witness address if an uncompressed key is used | ||
| # note that no witness address should be returned by unsolvable addresses | ||
| # the multisig_without_privkey_address will fail because its keys were not added with importpubkey |
sipa
referenced
this pull request
Jul 21, 2017
Closed
[RPC] Make ImportAddress works with segwit scriptPubKey #10891
laanwj
added this to the
0.15.0
milestone
Jul 25, 2017
|
@sdaftuar Would an explicit VerifyScript call (with consensus + compressed pubkey flags) after ProduceSignature help alleviate your concerns? |
|
I have updated this with @sipa's suggestion and squashed. |
achow101 commentedJul 11, 2017
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.