rpc/wallet: Add details and duplicate section for simulaterawtransaction #25621

pull anibilthare wants to merge 2 commits into bitcoin:master from anibilthare:202108-analyzerawtransaction changing 4 files +363 −43
  1. anibilthare commented at 5:15 PM on July 15, 2022: none

    The currently provided solution implements a RPC that iterates over the provided list of raw transactions and provides users with the balance change that the transactions will cause in the wallet but the current implementation does not deal with the conflicts that might arise while executing the transactions. This implementation is a follow up for the same, this implementation provides following enhancements to the RPC - For every transaction, say tx1, in the input list, a list of unconfirmed transactions in the wallet which conflict with tx1 - For every transaction, say tx2, in the input list, a list of transactions in the same list which conflict with tx2 - In case the user provides a transaction more than once, after the first occurrence, the rest of the occurrences are ignored and the index(0-based) for such transactions is reported to the user See also: PR#22751

  2. DrahtBot commented at 1:41 AM on July 16, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK josibake

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. anibilthare force-pushed on Jul 16, 2022
  4. fanquake added the label Wallet on Jul 19, 2022
  5. fanquake added the label RPC/REST/ZMQ on Jul 19, 2022
  6. anibilthare force-pushed on Jul 22, 2022
  7. in src/wallet/rpc/wallet.cpp:624 in bf9deb407a outdated 
     641 | @@ -621,11 +642,8 @@ RPCHelpMan simulaterawtransaction()
 642 |      std::shared_ptr<const CWallet> const wallet = GetWalletForJSONRPCRequest(request);
 643 |      if (!wallet) return NullUniValue;
 644 |      const CWallet* pwallet = wallet.get();
 645 | -
    achow101 commented at 6:25 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  8. in src/wallet/rpc/wallet.cpp:626 in bf9deb407a outdated 
     641 | @@ -621,11 +642,8 @@ RPCHelpMan simulaterawtransaction()
 642 |      std::shared_ptr<const CWallet> const wallet = GetWalletForJSONRPCRequest(request);
 643 |      if (!wallet) return NullUniValue;
 644 |      const CWallet* pwallet = wallet.get();
 645 | -
 646 |      RPCTypeCheck(request.params, {UniValue::VARR, UniValue::VOBJ}, true);
 647 | -
    achow101 commented at 6:25 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  9. in src/wallet/rpc/wallet.cpp:628 in bf9deb407a outdated 
     641 | @@ -621,11 +642,8 @@ RPCHelpMan simulaterawtransaction()
 642 |      std::shared_ptr<const CWallet> const wallet = GetWalletForJSONRPCRequest(request);
 643 |      if (!wallet) return NullUniValue;
 644 |      const CWallet* pwallet = wallet.get();
 645 | -
 646 |      RPCTypeCheck(request.params, {UniValue::VARR, UniValue::VOBJ}, true);
 647 | -
 648 |      LOCK(pwallet->cs_wallet);
 649 | -
    achow101 commented at 6:25 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  10. in src/wallet/rpc/wallet.cpp:640 in bf9deb407a outdated 
     654 | @@ -637,68 +655,136 @@ RPCHelpMan simulaterawtransaction()
 655 |  
 656 |          include_watchonly = options["include_watchonly"];
 657 |      }
 658 | -
    achow101 commented at 6:25 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  11. in src/wallet/rpc/wallet.cpp:703 in bf9deb407a outdated 
     717 | -                if (!coins.count(op)) {
 718 | -                    throw JSONRPCError(RPC_INVALID_PARAMETER, "One or more transaction inputs are missing");
 719 | -                }
 720 | -                if (coins.at(op).IsSpent()) {
 721 | -                    throw JSONRPCError(RPC_INVALID_PARAMETER, "One or more transaction inputs have been spent already");
 722 | +            // if user tries to double spend in the provided transactions themselves then without this condition we will get an inappropirate error
    achow101 commented at 6:33 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    This comment is a little confusing to understand.

                // Only apply changes the first time a transaction is seen. Otherwise duplicate transactions will give incorrect errors.
  12. in src/wallet/wallet.cpp:578 in bf9deb407a outdated 
     577 |          for (TxSpends::const_iterator _it = range.first; _it != range.second; ++_it)
 578 |              result.insert(_it->second);
 579 |      }
 580 |      return result;
 581 |  }
 582 | -
    achow101 commented at 6:37 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  13. in src/wallet/wallet.cpp:555 in bf9deb407a outdated 
     544 | @@ -545,7 +545,6 @@ void CWallet::SetMinVersion(enum WalletFeature nVersion, WalletBatch* batch_in)
 545 |              delete batch;
 546 |      }
 547 |  }
 548 | -
    achow101 commented at 6:37 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  14. in src/wallet/wallet.cpp:569 in bf9deb407a outdated 
     554 | @@ -556,19 +555,26 @@ std::set<uint256> CWallet::GetConflicts(const uint256& txid) const
 555 |          return result;
 556 |      const CWalletTx& wtx = it->second;
 557 |  
 558 | +    result = GetConflicts(*(wtx.tx), true);
 559 | +
 560 | +    return result;
 561 | +}
 562 | +// tx_is_in_wallet being true means the transaction belongs to wallet and tx_is_in_wallet being false means it is an external transaction
 563 | +std::set<uint256> CWallet::GetConflicts(const CTransaction& tx, bool tx_is_in_wallet) const
    achow101 commented at 6:37 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    This refactor of GetConflicts could be in a separate commit that precedes this commit.

  15. in src/wallet/wallet.cpp:560 in bf9deb407a outdated 
     554 | @@ -556,19 +555,26 @@ std::set<uint256> CWallet::GetConflicts(const uint256& txid) const
 555 |          return result;
 556 |      const CWalletTx& wtx = it->second;
 557 |  
 558 | +    result = GetConflicts(*(wtx.tx), true);
 559 | +
 560 | +    return result;
    achow101 commented at 6:38 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    result is not needed here.

        return Getconflicts(*wtx.tx, true);
  16. in test/functional/wallet_simulaterawtx.py:49 in bf9deb407a outdated 
      45 | @@ -46,41 +46,87 @@ def run_test(self):
  46 |  
  47 |          # Add address1 as watch-only to w2
  48 |          w2.importpubkey(pubkey=w1.getaddressinfo(address1)["pubkey"])
  49 | -
    achow101 commented at 6:38 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  17. in test/functional/wallet_simulaterawtx.py:56 in bf9deb407a outdated 
      51 |          tx2 = node.createrawtransaction([], [{address2: 10.0}])
  52 |  
  53 |          # w0 should be unaffected, w2 should see +5 for tx1
  54 |          assert_equal(w0.simulaterawtransaction([tx1])["balance_change"], 0.0)
  55 |          assert_equal(w2.simulaterawtransaction([tx1])["balance_change"], 5.0)
  56 | -
    achow101 commented at 6:38 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Unrelated whitespace change

  18. in test/functional/wallet_simulaterawtx.py:111 in bf9deb407a outdated 
     110 |          tx1 = funding["hex"]
 111 |          tx1changepos = funding["changepos"]
 112 |          bitcoin_fee = Decimal(funding["fee"])
 113 | -
 114 | -        # w0 sees fee + 5 btc decrease, w2 sees + 5 btc
 115 | +        # w0 sees (fee + 5) btc decrease, w2 sees + 5 btc
    achow101 commented at 6:39 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    Unrelated change

  19. in test/functional/wallet_simulaterawtx.py:172 in bf9deb407a outdated 
     166 | @@ -125,5 +167,183 @@ def run_test(self):
 167 |          assert_raises_rpc_error(-8, "One or more transaction inputs have been spent already", w1.simulaterawtransaction, [tx1, tx2])
 168 |          assert_raises_rpc_error(-8, "One or more transaction inputs have been spent already", w2.simulaterawtransaction, [tx1, tx2])
 169 |  
 170 | +
 171 | +
    achow101 commented at 6:40 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Extra blank lines can be removed.

  20. in test/functional/wallet_simulaterawtx.py:199 in bf9deb407a outdated 
     194 | +        ]
 195 | +        sim_result = w0.simulaterawtransaction([tx2, tx2, tx2])
 196 | +        assert_equal(sim_result["details"], details)
 197 | +        assert_equal(sim_result["duplicates"], duplicates)
 198 | +
 199 | +
    achow101 commented at 6:40 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Extra blank lines can be removed.

  21. in test/functional/wallet_simulaterawtx.py:223 in bf9deb407a outdated 
     219 | +            },
 220 | +        ]
 221 | +        sim_result = w0.simulaterawtransaction([tx2, tx3, tx4])
 222 | +        assert_equal(sim_result["details"], details)
 223 | +        assert_equal(sim_result["duplicates"], [])
 224 | +
    achow101 commented at 6:40 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    nit: Extra blank lines can be removed.

  22. in src/wallet/rpc/wallet.cpp:620 in bf9deb407a outdated 
     615 | +                    {RPCResult::Type::OBJ, "", "",
 616 | +                        {
 617 | +                            {RPCResult::Type::STR_HEX, "txid", "the transaction hex of transaction provided by the user"},
 618 | +                            {RPCResult::Type::ARR, "wallet_conflicts", "list of txids of transactions in the wallet that conflict with this transaction",
 619 | +                            {
 620 | +                                {RPCResult::Type::STR_HEX, "txid_in_wallet", "the txid of conflicting transactions in wallet"},
    achow101 commented at 6:43 PM on July 26, 2022:

    In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"

    txid is sufficient

                                    {RPCResult::Type::STR_HEX, "txid", "the txid of conflicting transactions in wallet"},
  23. achow101 commented at 6:44 PM on July 26, 2022: member

    Note: @anibilthare is my Summer of Bitcion mentee.

  24. anibilthare force-pushed on Aug 1, 2022
  25. achow101 commented at 7:34 PM on August 5, 2022: member

    Can be rebased now that #22751 is merged.

  26. DrahtBot added the label Needs rebase on Aug 5, 2022
  27. in src/wallet/rpc/wallet.cpp:625 in c5087b3be6 outdated 
     620 | +                                {RPCResult::Type::STR_HEX, "txid", "the txid of conflicting transactions in wallet"},
 621 | +                            }},
 622 | +                            {RPCResult::Type::ARR, "simulated_tx_conflicts", "indices of transactions provided by the user that conflict with this transaction",
 623 | +                            {
 624 | +                                {RPCResult::Type::NUM, "index", "index of conflicting transactions in list provided by user"},
 625 | +                            }},
    josibake commented at 9:42 AM on August 11, 2022:

    Why are we returning indices in simulated_tx_conflicts instead of txids? As an end user, I think I'd prefer to have a consistent schema for the details object where both *_conflicts fields have txids.

    anibilthare commented at 6:03 AM on August 21, 2022:

    Returning txids increases the "manual" work on user's side, if that makes sense? Consider a case when user provides 50 transactions as input. As an end user I think I'd prefer the software telling me "This transaction of yours conflicts with the transaction 11, 17 and 21 that you provided" rather than "This transaction of yours conflict with the transactions with txids <some-long-string1> <some-long-string2> <some-long-string3>"

  28. in src/wallet/rpc/wallet.cpp:631 in c5087b3be6 outdated 
     626 | +                        }
 627 | +                    },
 628 | +                }},
 629 | +                {RPCResult::Type::ARR, "duplicates", "indices of transactions provided by the user more than once and hence ignored",
 630 | +                {
 631 | +                    {RPCResult::Type::NUM, "index", "index of transactions provided by the user more than once"},
    josibake commented at 9:44 AM on August 11, 2022:

    I don't understand why this is needed. Wouldn't it be better to have the RPC fail fast and tell the user they are providing duplicates?

    anibilthare commented at 5:48 AM on August 21, 2022:

    I did consider what you suggested but let's say we do that, then the user will remove those duplicate transactions from the list and re run the RPC? To me this seemed redundant and that's why I took this approach.

  29. in src/wallet/rpc/wallet.cpp:666 in c5087b3be6 outdated 
     661 | @@ -642,63 +662,132 @@ RPCHelpMan simulaterawtransaction()
 662 |      if (ParseIncludeWatchonly(include_watchonly, *pwallet)) {
 663 |          filter |= ISMINE_WATCH_ONLY;
 664 |      }
 665 | -
 666 | +    // fetch transactions
    josibake commented at 9:47 AM on August 11, 2022:

    nit: adding comments which restate what existing code is doing aren't super valuable imo and can be distracting when trying to review the code changes

  30. in src/wallet/rpc/wallet.cpp:685 in c5087b3be6 outdated 
     683 | +    std::map<int, uint256> txhash;
 684 | +    // used for dealing with duplicates
 685 | +    std::map<uint256, bool> done;
 686 |      for (size_t i = 0; i < txs.size(); ++i) {
 687 |          CMutableTransaction mtx;
 688 | +        // decoding ith transaction hex
    josibake commented at 9:48 AM on August 11, 2022:

    same nit re: comments

  31. josibake commented at 10:04 AM on August 11, 2022: member

    Concept ACK

    Nice work with informing the user of conflicts! Seems to definitely improve the usability of the RPC. I tried rebasing on master and the rebase wasn't trivial, so I'll wait until you're able to rebase to do a more thorough code review. I left a few comments on the design of the details object and a few nits on the code. In general, I'd avoid making stylistic changes (adding/removing whitespace) and avoid adding/editing comments on the existing code.

    Lastly, it might help to break this up into a few smaller commits to make it easier to review. I'd suggest putting the refactors (renaming variables, refactoring GetConflicts) into their own refactor commit and a separate commit for the new functionality.

  32. Refactored GetConflicts c428f6e14f
  33. rpc/wallet: Add details and duplicate section for simulaterawtransaction
    The currently provided solution implements an RPC that iterates over the provided list of raw transactions and provides users with the balance change that the transactions will cause in the wallet but the current implementation does not deal with the conflicts that might arise while executing the transactions. This implementation is a follow up for the same, this implementation provides following enhancement to the RPC
	- For every transaction, say tx1, in the input list, a list of unconfirmed transactions in the wallet which conflict with tx1
	- For every transaction, say tx2, in the input list, a list of transactions in the same list which conflict with tx2
	- In case the user provides a transaction more than once, after first occurrence, rest occurrences are ignored and the index(0-based) for such transactions is reported to the user

See also: PR#22751
    d96937042c
  34. anibilthare force-pushed on Aug 26, 2022
  35. DrahtBot removed the label Needs rebase on Aug 26, 2022
  36. fanquake commented at 2:57 PM on February 17, 2023: member

    @anibilthare are you still working on this? Can you comment where you've addressed the review feedback. Note that at least one test is also failing.

  37. maflcko added the label Needs rebase on Feb 17, 2023
  38. DrahtBot removed the label Needs rebase on Feb 17, 2023
  39. DrahtBot renamed this:
    rpc/wallet: Add details and duplicate section for simulaterawtransaction
    rpc/wallet: Add details and duplicate section for simulaterawtransaction
    on Feb 17, 2023
  40. achow101 closed this on Apr 25, 2023
  41. bitcoin locked this on Apr 24, 2024
Contributors
anibilthareDrahtBotachow101josibakefanquake
Labels
WalletRPC/REST/ZMQ
Linked (view graph)
#22751 rpc/wallet: add simulaterawtransaction RPC
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me