Bumpfee: bugfixes for error handling and feerate calculation #9640

1. 
   sdaftuar commented at 8:58 PM on January 26, 2017: member

   Please tag 0.14.

   This is built on #9589 -- the last 3 commits here are new.

   The first fix is a minor one, which is to correctly calculate the maximum size of the replacement transaction, which is used to calculate the fee to put on the transaction. The old algorithm (of adding 1 byte per input) is actually not always conservative enough, for instance in situations where you get "lucky" and the original transaction has very short signatures. I managed to encounter such a transaction during testing of #9589, and it was a difficult-to-debug problem, so I thought we should fix.

   The second fix is a more important bugfix, which is that if the replacement transaction failed to enter the mempool (#9589 makes this much less likely to happen, fortunately), then we'd throw an RPC error even though the new transaction has been recorded in the wallet (and not mark the original tx as having been replaced). This is similar to the bug we fixed in #9302 -- we should always return the new txid to the user once we've generated it, as throwing an error implies the operation failed. The approach I took was to aggregate the errors in a single JSON object and return it, similar to what we do in some other RPCs.

2. MarcoFalke added this to the milestone 0.14.0 on Jan 26, 2017
3. MarcoFalke added the label Wallet on Jan 26, 2017
4. MarcoFalke added the label RPC/REST/ZMQ on Jan 26, 2017
5. 
   morcos commented at 2:46 AM on January 27, 2017: member

   concept ACK i think the last commit at least is important to have to 0.14..

6. in src/wallet/rpcwallet.cpp:None in b4f67cb950 outdated

   2633 | @@ -2634,6 +2634,31 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
   2634 |      return result;
   2635 |  }
   2636 |  
   2637 | +// Calculate the size of the transaction assuming all signatures are max size
   2638 | +// Use DummySignatureCreator, which inserts 72 byte signatures everywhere.
   2639 | +// TODO: re-use this in CWalletTx::CreateTransaction (right now now
   

   ---

   ---

   

   ryanofsky commented at 5:02 PM on January 27, 2017:

   Can remove this todo? Seems to be taken care of in previous commit.

   ---

   

   sdaftuar commented at 5:14 PM on January 27, 2017:

   Oh I had originally tried to write this so that CalculateMaximumSignedTxSize() would be a function in CWallet, and CreateTransaction would directly use it -- but that was harder than I expected since CreateTransaction actually uses the dummy-signed tx for something that bumpfee doesn't care about (the priority calculation I mentioned).

   So this todo refers to making these code paths even more similar than they are now.

   ---

   

   TheBlueMatt commented at 7:01 PM on January 31, 2017:

   Yes, lets not forget to do this after #9602

7. 
   laanwj commented at 11:35 AM on January 30, 2017: member

   Needs rebase, I think the merge of #9615 introduced conflicts

8. wallet: Refactor dummy signature signing for reusability d625b907a1
9. sdaftuar force-pushed on Jan 30, 2017
10. 
    sdaftuar commented at 7:03 PM on January 30, 2017: member

    Rebased.

11. 
    MarcoFalke commented at 7:15 PM on January 30, 2017: member

    utACK 1fa55fca3b206df38dc4bd15e5cbe697e6adade1

12. 
    laanwj commented at 11:17 AM on January 31, 2017: member

    The approach I took was to aggregate the errors in a single JSON object and return it, similar to what we do in some other RPCs.

    So this changes the way to detect wheter an error happened from looking at the JSON-RPC-level error to seeing if rv["error"] is a non-empty list?

    I guess this change in interface is acceptable as bumpfee is new for 0.14 and it's unlikely that any code will be relying on the old behavior.

13. 
    sdaftuar commented at 2:51 PM on January 31, 2017: member

    The approach I took was to aggregate the errors in a single JSON object and return it, similar to what we do in some other RPCs.

    So this changes the way to detect wheter an error happened from looking at the JSON-RPC-level error to seeing if rv["error"] is a non-empty list? @laanwj Only for those errors which occur after the rpc call has successfully created a new transaction, so that the user doesn't think the RPC failed when it actually succeeded. (For errors that prevent the creation of a new transaction, a JSON-RPC exception is returned as before.)

14. 
    MarcoFalke commented at 4:00 PM on January 31, 2017: member

    Indeed, we don't want to reintroduce the bug we just fixed in 0.13.2, so the last commit is required for 0.14.0.

15. in src/wallet/rpcwallet.cpp:None in 1fa55fca3b outdated

    2670 | +// CreateTransaction uses the constructed dummy-signed tx to do a priority
    2671 | +// calculation, but we should be able to refactor after priority is removed).
    2672 | +int64_t CalculateMaximumSignedTxSize(const CTransaction &tx)
    2673 | +{
    2674 | +    CMutableTransaction txNew(tx);
    2675 | +    std::vector<pair<CWalletTx *, int>> vCoins;
    

    ---

    ---

    

    TheBlueMatt commented at 7:17 PM on January 31, 2017:

    nit: Technically the int should be unsigned?

16. in src/wallet/rpcwallet.cpp:None in 1fa55fca3b outdated

    2663 | @@ -2664,6 +2664,31 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
    2664 |      return result;
    2665 |  }
    2666 |  
    2667 | +// Calculate the size of the transaction assuming all signatures are max size
    2668 | +// Use DummySignatureCreator, which inserts 72 byte signatures everywhere.
    

    ---

    ---

    

    TheBlueMatt commented at 7:26 PM on January 31, 2017:

    Might want to add an additional comment pointing out that ALL inputs MUST be in mapWallet (eg IsAllFromMe, which we do check in bumpfee).

17. 
    TheBlueMatt commented at 7:57 PM on January 31, 2017: member

    utACK 1fa55fca3b206df38dc4bd15e5cbe697e6adade1

18. 
    sdaftuar commented at 8:24 PM on January 31, 2017: member

    Added a fixup commit to address @TheBlueMatt's nits.

19. 
    TheBlueMatt commented at 8:37 PM on January 31, 2017: member

    re-utACK 227f50a373c6c7af9e5b6f8a85875ffa7c2c4be7 (post-squash)

20. rpc: bumpfee: use correct maximum signed tx size for fee calculation

    More accurate than simply adding one byte per input, and properly handles the
    case where the original transaction happened to have very small signatures

    f62659448c
21. rpc: bumpfee: handle errors more gracefully 9522b53a91
22. sdaftuar force-pushed on Feb 1, 2017
23. 
    sdaftuar commented at 1:06 AM on February 1, 2017: member

    Squashed 227f50a -> 9522b53a91f28032c34b94662d50b000534708ce

24. 
    jtimon commented at 2:22 AM on February 1, 2017: contributor

    utACK individual commits: d625b907a1800a5a30c4ad285641c7418d2c28c1 9522b53a91f28032c34b94662d50b000534708ce

25. 
    laanwj commented at 7:42 AM on February 1, 2017: member

    utACK 9522b53

26. laanwj merged this on Feb 1, 2017
27. laanwj closed this on Feb 1, 2017

    ---

28. laanwj referenced this in commit 7bfb77045c on Feb 1, 2017
29. HashUnlimited referenced this in commit 1d2c32110e on Feb 27, 2018
30. DrahtBot locked this on Sep 8, 2021

Contributors

sdaftuarmorcosryanofskyTheBlueMattlaanwjMarcoFalkejtimon

Milestone
0.14.0