Fix importmulti bug when importing an already imported key #11483

1. 
   pedrobranco commented at 10:20 AM on October 11, 2017: contributor

   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" } }]
   

2. fanquake added the label RPC/REST/ZMQ on Oct 11, 2017
3. in src/wallet/rpcdump.cpp:966 in 5e3c7c4f2c outdated

    962 | @@ -961,7 +963,8 @@ UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int6
    963 |                  pwallet->SetAddressBook(vchAddress, label, "receive");
    964 |  
    965 |                  if (pwallet->HaveKey(vchAddress)) {
    966 | -                    return false;
    967 | +                    result.pushKV("success", UniValue(true));
   

   ---

   ---

   

   promag commented at 10:56 AM on October 11, 2017:

   Should be true?

   ---

   

   pedrobranco commented at 11:24 AM on October 11, 2017:

   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.

   ---

   

   promag commented at 11:39 AM on October 11, 2017:

   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");

   ---

   

   promag commented at 11:41 AM on October 11, 2017:

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

   ---

   

   pedrobranco commented at 1:08 PM on October 11, 2017:

   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.

4. 
   ryanofsky commented at 11:42 AM on October 11, 2017: member

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

5. 
   pedrobranco commented at 1:10 PM on October 11, 2017: contributor

   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 }]
   

6. pedrobranco force-pushed on Oct 11, 2017
7. in test/functional/importmulti.py:63 in 21f75f90cc outdated

     58 | +            "scriptPubKey": {
     59 | +                "address": address['address']
     60 | +            },
     61 | +            "timestamp": "now",
     62 | +        }])
     63 | +        assert_equal(result[0]['success'], True)
   

   ---

   ---

   

   ryanofsky commented at 3:17 PM on October 11, 2017:

   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.

   ---

   

   pedrobranco commented at 4:10 PM on October 11, 2017:

   I'm dropping this test.

8. 
   ryanofsky commented at 3:20 PM on October 11, 2017: member

   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.

9. pedrobranco force-pushed on Oct 11, 2017
10. pedrobranco force-pushed on Oct 11, 2017
11. Fix importmulti bug when importing an already imported key a44a215177
12. pedrobranco force-pushed on Oct 11, 2017
13. 
    ryanofsky commented at 5:42 PM on October 11, 2017: member

    Thanks! utACK a44a215177ab55b4a3b36a7980c313e908e2dd18. Only changed is dropped test.

14. 
    pedrobranco commented at 3:03 PM on October 17, 2017: contributor

    Any thoughts when this can be merged?

15. 
    ryanofsky commented at 3:27 PM on October 17, 2017: member

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

16. jonasschnelli added the label Needs backport on Oct 17, 2017
17. 
    jonasschnelli commented at 4:14 PM on October 17, 2017: contributor

    utACK a44a215177ab55b4a3b36a7980c313e908e2dd18 Added "Needs back port" tag.

18. 
    TheBlueMatt commented at 5:27 PM on October 17, 2017: member

    utACK a44a215177ab55b4a3b36a7980c313e908e2dd18

19. MarcoFalke referenced this in commit 20cdc2b36c on Oct 17, 2017
20. MarcoFalke merged this on Oct 17, 2017
21. MarcoFalke closed this on Oct 17, 2017

    ---

22. MarcoFalke referenced this in commit 808c84f89d on Oct 17, 2017
23. 
    MarcoFalke commented at 7:46 PM on October 17, 2017: member

    utACK a44a215

    (Backport label can be removed)

24. 
    promag commented at 9:31 PM on October 17, 2017: member

    utACK a44a215.

25. pedrobranco deleted the branch on Oct 18, 2017
26. MarcoFalke removed the label Needs backport on Oct 19, 2017
27. MarcoFalke added this to the milestone 0.15.0.2 on Oct 19, 2017
28. codablock referenced this in commit f616989371 on Sep 25, 2019
29. barrystyle referenced this in commit e7aa52e223 on Jan 22, 2020
30. DrahtBot locked this on Sep 8, 2021

Contributors

pedrobrancopromagryanofskyjonasschnelliTheBlueMattMarcoFalke

Milestone
0.15.1