Fix importmulti bug when importing an already imported key #11483

pedrobranco · 2017-10-11T10:20:02Z

This PR fixes a bug in importmulti RPC call where it returns an invalid response when importing an already imported key.

Before:

❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]'
[{ "success": true }]

❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' '{ "rescan": false }'
[ false ]

❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' '{ "rescan": true }'
error code: -1
error message:
JSON value is not a boolean as expected

After this fix:

❯ bitcoin-cli -rpcuser=u -rpcpassword=p -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655139 }]'
[{ "success": true }]

❯ bitcoin-cli -rpcuser=u -rpcpassword=p -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655139 }]'
[{ "success": false, "error": { "code": -4, "message": "The wallet already contains the private key for this address or script" } }]

promag · 2017-10-11T10:56:51Z

src/wallet/rpcdump.cpp

Should be true?

Since when success is false returns the associated error, in this situation since there's no associated error, success should be true. Just like calling n times the importprivkey RPC for a given private key: it does not throw any error.

Right, but maybe that behaviour is wrong? Search for throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");

BTW, all previous exit points are exceptions, this is the only early return.

Right, but maybe that behaviour is wrong? Search for throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");

Right, so we can keep it consistent with the rest of the importmulti logic by throwing that error.

ryanofsky · 2017-10-11T11:42:56Z

This seems like a good change, but why are you calling it a bugfix? Can you add a description of the bug?

pedrobranco · 2017-10-11T13:10:46Z

This seems like a good change, but why are you calling it a bugfix? Can you add a description of the bug?

It's a bug because we should receive an array of JSON responses.
In this situation we are receiving an array with a boolean:

❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]'
[ { "success": true } ]

❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' '{ "rescan": false }'
[ false ] # expected [{ "success": false }]

ryanofsky

Conditional ACK 21f75f90cceeea23995436e6d0f107e265e224ec if new 'Should not import an already imported address' test is either removed or moved to a separate commit, or explained in relation to the bugfix.

ryanofsky · 2017-10-11T15:17:23Z

test/functional/importmulti.py

The test below ("Should not import an address with private key if is already imported") seems good, but this test ("Should not import an already imported address") is confusing to me.

For one thing, it doesn't seem to have anything to do with the bug, because it passes both before and after the bugfix. But also the description seems misleading because it is checking that success is true while claiming the address shouldn't be imported.

Would suggest either dropping this new test, or moving it to a separate commit where it is clear this has nothing to do with the bugfix. And the test description should be clarified it it will be kept.

I'm dropping this test.

ryanofsky

Thanks! utACK a44a215. Only changed is dropped test.

pedrobranco · 2017-10-17T15:03:53Z

Any thoughts when this can be merged?

ryanofsky · 2017-10-17T15:27:33Z

Looks ready to me. Mentioning @laanwj to maybe include in the next batch of merges.

jonasschnelli · 2017-10-17T16:14:00Z

utACK a44a215
Added "Needs back port" tag.

TheBlueMatt · 2017-10-17T17:27:39Z

utACK a44a215

Github-Pull: bitcoin#11483 Rebased-From: a44a215

a44a215 Fix importmulti bug when importing an already imported key (Pedro Branco) Pull request description: This PR fixes a bug in `importmulti` RPC call where it returns an invalid response when importing an already imported key. Before: ```sh ❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' [{ "success": true }] ❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' '{ "rescan": false }' [ false ] ❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' '{ "rescan": true }' error code: -1 error message: JSON value is not a boolean as expected ``` After this fix: ```sh ❯ bitcoin-cli -rpcuser=u -rpcpassword=p -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655139 }]' [{ "success": true }] ❯ bitcoin-cli -rpcuser=u -rpcpassword=p -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655139 }]' [{ "success": false, "error": { "code": -4, "message": "The wallet already contains the private key for this address or script" } }] ``` Tree-SHA512: 4acebdfb7d0ebd7cd48e943b93ed1cec072db1ace5c42b3f5cc225603764b6e804e4b823b0710965826aafc2f0c615c53d5aefcfdb9bc9c379f5221b798a318c

maflcko · 2017-10-17T19:46:34Z

utACK a44a215 (Backport label can be removed)

promag · 2017-10-17T21:31:10Z

utACK a44a215.

…ported key a44a215 Fix importmulti bug when importing an already imported key (Pedro Branco) Pull request description: This PR fixes a bug in `importmulti` RPC call where it returns an invalid response when importing an already imported key. Before: ```sh ❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' [{ "success": true }] ❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' '{ "rescan": false }' [ false ] ❯ bitcoin-cli -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655239 }]' '{ "rescan": true }' error code: -1 error message: JSON value is not a boolean as expected ``` After this fix: ```sh ❯ bitcoin-cli -rpcuser=u -rpcpassword=p -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655139 }]' [{ "success": true }] ❯ bitcoin-cli -rpcuser=u -rpcpassword=p -regtest importmulti '[{ "keys": ["cNcMUunXhVK1dXJ5riixtpYSxPXZnUAMGS4vpzwChdKmYY3Rz99v"], "scriptPubKey": { "address": "n4YZAf4WE2XF3t4BfeYS2nHAhb8CVx91BR" }, "timestamp": 1507655139 }]' [{ "success": false, "error": { "code": -4, "message": "The wallet already contains the private key for this address or script" } }] ``` Tree-SHA512: 4acebdfb7d0ebd7cd48e943b93ed1cec072db1ace5c42b3f5cc225603764b6e804e4b823b0710965826aafc2f0c615c53d5aefcfdb9bc9c379f5221b798a318c

fanquake added the RPC/REST/ZMQ label Oct 11, 2017

promag reviewed Oct 11, 2017

View reviewed changes

pedrobranco force-pushed the bugfix/fix-importmulti-bug branch from 5e3c7c4 to 21f75f9 Compare October 11, 2017 13:33

ryanofsky reviewed Oct 11, 2017

View reviewed changes

pedrobranco force-pushed the bugfix/fix-importmulti-bug branch 2 times, most recently from f4e9446 to 637a338 Compare October 11, 2017 16:19

Fix importmulti bug when importing an already imported key

a44a215

pedrobranco force-pushed the bugfix/fix-importmulti-bug branch from 637a338 to a44a215 Compare October 11, 2017 16:21

ryanofsky reviewed Oct 11, 2017

View reviewed changes

jonasschnelli added the Needs backport label Oct 17, 2017

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 17, 2017

Fix importmulti bug when importing an already imported key

20cdc2b

Github-Pull: bitcoin#11483 Rebased-From: a44a215

maflcko merged commit a44a215 into bitcoin:master Oct 17, 2017

pedrobranco deleted the bugfix/fix-importmulti-bug branch October 18, 2017 09:39

pierreN mentioned this pull request Oct 18, 2017

Compile fails on recent ubuntu / boost 1.64 #11520

Closed

maflcko removed the Needs backport label Oct 19, 2017

maflcko added this to the 0.15.0.2 milestone Oct 19, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix importmulti bug when importing an already imported key #11483

Fix importmulti bug when importing an already imported key #11483

Uh oh!

Conversation

pedrobranco commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

pedrobranco Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

promag Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

pedrobranco Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 11, 2017

Uh oh!

pedrobranco commented Oct 11, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

pedrobranco Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

pedrobranco commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 17, 2017

Uh oh!

jonasschnelli commented Oct 17, 2017

Uh oh!

TheBlueMatt commented Oct 17, 2017

Uh oh!

maflcko commented Oct 17, 2017 via email

Uh oh!

promag commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pedrobranco commented Oct 11, 2017 •

edited

Loading

pedrobranco Oct 11, 2017 •

edited

Loading

pedrobranco commented Oct 17, 2017 •

edited

Loading