refactor: Make CWalletTx sync state type-safe #21206

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/txstate changing 16 files +257 −174
  1. ryanofsky commented at 5:08 AM on February 17, 2021: contributor

    Current CWalletTx state representation makes it possible to set inconsistent states that won't be handled correctly by wallet sync code or serialized & deserialized back into the same form.

    For example, it is possible to call setConflicted without setting a conflicting block hash, or setConfirmed with no transaction index. And it's possible update individual m_confirm and fInMempool data fields without setting an overall consistent state that can be serialized and handled correctly.

    Fix this without changing behavior by using std::variant, instead of an enum and collection of fields, to represent sync state, so state tracking code is safer and more legible.

    This is a first step to fixing state tracking bugs https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, by adding an extra margin of safety that can prevent new bugs from being introduced as existing bugs are fixed.

  2. fanquake added the label Refactoring on Feb 17, 2021
  3. fanquake added the label Wallet on Feb 17, 2021
  4. ryanofsky referenced this in commit 2a855bf092 on Feb 17, 2021
  5. ryanofsky force-pushed on Feb 17, 2021
  6. ryanofsky cross-referenced this on Feb 17, 2021 from issue MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky
  7. DrahtBot commented at 10:25 AM on February 17, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)
    • #23411 (refactor: Avoid integer overflow in ApplyStats when activating snapshot by MarcoFalke)

    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.

  8. DrahtBot cross-referenced this on Feb 17, 2021 from issue rpc: Disallow sendtoaddress and sendmany when private keys disabled by achow101
  9. DrahtBot cross-referenced this on Feb 17, 2021 from issue wallet: Avoid requesting fee rates multiple times during coin selection by achow101
  10. DrahtBot cross-referenced this on Feb 17, 2021 from issue bitcoind: Add -daemonwait option to wait for initialization by laanwj
  11. DrahtBot cross-referenced this on Feb 17, 2021 from issue refactor: split CWallet::Create by S3RK
  12. DrahtBot cross-referenced this on Feb 17, 2021 from issue wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack
  13. DrahtBot cross-referenced this on Feb 17, 2021 from issue wallet, bugfix: fix ComputeTimeSmart function during rescanning process. by BitcoinTsunami
  14. DrahtBot cross-referenced this on Feb 17, 2021 from issue wallet: Error with "Transaction too large" if the funded tx will end up being too large after signing by achow101
  15. DrahtBot cross-referenced this on Feb 17, 2021 from issue util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift
  16. DrahtBot cross-referenced this on Feb 17, 2021 from issue Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files by practicalswift
  17. DrahtBot cross-referenced this on Feb 17, 2021 from issue rpc, wallet: Expose wallet id in getwalletinfo RPC output by hebasto
  18. DrahtBot cross-referenced this on Feb 17, 2021 from issue wallet: Properly support a wallet id by achow101
  19. DrahtBot cross-referenced this on Feb 17, 2021 from issue wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 by fjahr
  20. DrahtBot cross-referenced this on Feb 18, 2021 from issue Add Single Random Draw as an additional coin selection algorithm by achow101
  21. DrahtBot cross-referenced this on Feb 18, 2021 from issue gui: grey out used address in address book by za-kk
  22. DrahtBot cross-referenced this on Feb 18, 2021 from issue Use effective values throughout coin selection by achow101
  23. DrahtBot cross-referenced this on Feb 18, 2021 from issue Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101
  24. DrahtBot cross-referenced this on Feb 18, 2021 from issue External signer support - Wallet Box edition by Sjors
  25. DrahtBot cross-referenced this on Feb 18, 2021 from issue rpc: Added ability to remove watch only addresses by benthecarman
  26. DrahtBot cross-referenced this on Feb 18, 2021 from issue [RPC] Include coinbase transactions in receivedby RPCs by andrewtoth
  27. DrahtBot added the label Needs rebase on Feb 19, 2021
  28. in src/wallet/transaction.h:136 in 7bc16a0b42 outdated
     133 | @@ -140,8 +134,7 @@ class CWalletTx
     134 |      mutable CAmount nChangeCached;
     135 |  
     136 |      CWalletTx(const CWallet* wallet, CTransactionRef arg)
    


    promag commented at 12:52 PM on February 21, 2021:

    7bc16a0b421380a1682ca9d97b7f7fc140fb1c5e

    nit, CWalletTx(const CWallet*, CTransactionRef arg) to make it clear wallet is unused.


    ryanofsky commented at 12:26 AM on March 4, 2021:

    re: #21206 (review)

    nit, CWalletTx(const CWallet*, CTransactionRef arg) to make it clear wallet is unused.

    Good catch! Removed unused argument in this commit instead of later commit

  29. promag commented at 12:56 PM on February 21, 2021: member

    Approach ACK (also base PR).

    This makes it a lot easier to reason about transaction state. Another great achievement is dropping wallet dependency from transaction. 👏

  30. ryanofsky referenced this in commit 11dd250fb1 on Mar 4, 2021
  31. ryanofsky referenced this in commit 01f4bb2693 on Mar 4, 2021
  32. ryanofsky force-pushed on Mar 4, 2021
  33. ryanofsky commented at 12:41 AM on March 4, 2021: contributor

    Rebased 792d72e2a17fa34e173a82171bdaee3ba88f3e74 -> a4bc501a00293a12f0f6d44d85ffb324823b472d (pr/txstate.2 -> pr/txstate.3, compare) on top of #21207 pr/txmove.2 Rebased a4bc501a00293a12f0f6d44d85ffb324823b472d -> 56aeb9ff7d5942fdb010db3773e26233e0d1612d (pr/txstate.3 -> pr/txstate.4, compare) on top of #21207 pr/txmove.3 Rebased 56aeb9ff7d5942fdb010db3773e26233e0d1612d -> 4c5f91eaf834e3eacc480c8d277e64aebc5ebd4e (pr/txstate.4 -> pr/txstate.5, compare) due to conflicts with #21331 and #18842 Rebased 4c5f91eaf834e3eacc480c8d277e64aebc5ebd4e -> ed04456421d8092457fbefd70b77c13a53a3d20f (pr/txstate.5 -> pr/txstate.6, compare) on top of #21207 pr/txmove.4 due to conflict with #21141 Rebased ed04456421d8092457fbefd70b77c13a53a3d20f -> 045e712a489ed8df4cc15f29a059c8f661759e72 (pr/txstate.6 -> pr/txstate.7, compare) on top of #21207 pr/txmove.5 Rebased 045e712a489ed8df4cc15f29a059c8f661759e72 -> 4e11f88320b644b67db55fe737815451ca7d0681 (pr/txstate.7 -> pr/txstate.8, compare) due to conflict with #21666

  34. DrahtBot removed the label Needs rebase on Mar 4, 2021
  35. DrahtBot cross-referenced this on Mar 4, 2021 from issue rpc: replace wallet raw pointers with references (#18592 rebased) by fanquake
  36. DrahtBot cross-referenced this on Mar 4, 2021 from issue wallet: remove lock during `listaddressgroupings` by vladyslavstartsev
  37. DrahtBot cross-referenced this on Mar 4, 2021 from issue Implement BIP 370 PSBTv2 by achow101
  38. DrahtBot cross-referenced this on Mar 4, 2021 from issue rpc: include_unsafe option for fundrawtransaction by t-bast
  39. DrahtBot cross-referenced this on Mar 4, 2021 from issue Basic Taproot signing support for descriptor wallets by sipa
  40. DrahtBot added the label Needs rebase on Mar 8, 2021
  41. ryanofsky referenced this in commit 74d45298e5 on Mar 8, 2021
  42. ryanofsky referenced this in commit 065e94fd97 on Mar 8, 2021
  43. ryanofsky force-pushed on Mar 8, 2021
  44. DrahtBot removed the label Needs rebase on Mar 9, 2021
  45. DrahtBot added the label Needs rebase on Mar 10, 2021
  46. ryanofsky referenced this in commit 68e865e6b7 on Mar 10, 2021
  47. ryanofsky force-pushed on Mar 10, 2021
  48. ryanofsky referenced this in commit 8da243cc98 on Mar 10, 2021
  49. DrahtBot removed the label Needs rebase on Mar 10, 2021
  50. DrahtBot cross-referenced this on Mar 11, 2021 from issue refactor: Remove MakeUnique<T>() by fanquake
  51. DrahtBot cross-referenced this on Mar 11, 2021 from issue refactor: remove Optional & nullopt by fanquake
  52. DrahtBot added the label Needs rebase on Mar 12, 2021
  53. ryanofsky referenced this in commit e7eebeb6d1 on Mar 21, 2021
  54. ryanofsky referenced this in commit 7c188f3508 on Mar 21, 2021
  55. ryanofsky force-pushed on Mar 21, 2021
  56. DrahtBot cross-referenced this on Mar 22, 2021 from issue Move external signer out of wallet module by Sjors
  57. DrahtBot removed the label Needs rebase on Mar 22, 2021
  58. DrahtBot cross-referenced this on Mar 31, 2021 from issue refactor: remove ::vpwallets and related global variables by ryanofsky
  59. DrahtBot cross-referenced this on Apr 3, 2021 from issue rpc, gui: bumpfee signer support by Sjors
  60. DrahtBot added the label Needs rebase on Apr 13, 2021
  61. ryanofsky referenced this in commit a71a7db9a9 on Apr 13, 2021
  62. ryanofsky referenced this in commit 9b6bdfb08a on Apr 13, 2021
  63. ryanofsky force-pushed on Apr 13, 2021
  64. DrahtBot removed the label Needs rebase on Apr 13, 2021
  65. DrahtBot cross-referenced this on Apr 13, 2021 from issue Miscellaneous external signer changes by fanquake
  66. DrahtBot added the label Needs rebase on Apr 14, 2021
  67. ryanofsky force-pushed on Apr 14, 2021
  68. DrahtBot removed the label Needs rebase on Apr 14, 2021
  69. DrahtBot cross-referenced this on Apr 22, 2021 from issue wallet: document coin selection code by glozow
  70. in src/wallet/interfaces.cpp:85 in 4e11f88320 outdated
      80 | @@ -81,7 +81,9 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
      81 |  WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx)
      82 |  {
      83 |      WalletTxStatus result;
      84 | -    result.block_height = wtx.m_confirm.block_height > 0 ? wtx.m_confirm.block_height : std::numeric_limits<int>::max();
      85 | +    int height = wtx.state<TxStateConfirmed>() ? wtx.state<TxStateConfirmed>()->confirmed_block_height :
    


    promag commented at 1:12 PM on April 26, 2021:

    4e11f88320b644b67db55fe737815451ca7d0681

    nit, refactor to wtx.GetBlockHeight() or similar.


    ryanofsky commented at 3:22 PM on April 30, 2021:

    re: #21206 (review)

    4e11f88

    nit, refactor to wtx.GetBlockHeight() or similar.

    The code does repeat a few places but I think I'd still rather not have this. I think all these positive and negative height and magic height and depth values are bad and we should just pass the actual state information around. It would be good to clean up this code later (would avoid it for now to avoid expanding scope of pr), but I think the cleanup should more try to get rid of the magic height values than to try to construct them quietly.

  71. in src/wallet/wallet.cpp:809 in 4e11f88320 outdated
     787 | @@ -788,7 +788,9 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
     788 |      // notification so the wallet is in an internally consistent state and
     789 |      // immediately knows the old transaction should not be considered trusted
     790 |      // and is eligible to be abandoned
     791 | -    wtx.fInMempool = chain().isInMempool(originalHash);
     792 | +    if (wtx.state<TxStateInMempool>() && !chain().isInMempool(originalHash)) {
    


    promag commented at 1:16 PM on April 26, 2021:

    4e11f88320b644b67db55fe737815451ca7d0681

    Why the first check?


    ryanofsky commented at 3:55 PM on April 30, 2021:

    re: #21206 (review)

    4e11f88

    Why the first check?

    The main reason is to be equivalent as possible to previous code. Previous code didn't change the transactions abandon/unconfirmed/conflicted state and this code isn't trying to do that either. Also, I think it makes the code more closely match the comment above. Maybe we can make the comment more clear though. It seems like it could be a little more specific but I'm not sure what extra explanation or details might be good to add.

  72. in src/wallet/wallet.cpp:1000 in 4e11f88320 outdated
     973 | -        if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) {
     974 | -            // Update cached block height variable since it not stored in the
     975 | -            // serialized transaction.
     976 | -            wtx.m_confirm.block_height = height;
     977 | -        } else if (wtx.isConflicted() || wtx.isConfirmed()) {
     978 | +        auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
    


    promag commented at 2:09 PM on April 26, 2021:

    4e11f88320b644b67db55fe737815451ca7d0681

    nit, lookup_block seems misleading, maybe update_tx_state or mark_inactive.


    ryanofsky commented at 4:18 PM on April 30, 2021:

    re: #21206 (review)

    4e11f88

    nit, lookup_block seems misleading, maybe update_tx_state or mark_inactive.

    I guess it's not too important because the definition is a few lines away from the usage, but those names seem more misleading to me. The lambda is looking up a block hash and returning the block height in an output parameter. In the error case it also returns a txstate. I think it's reasonable to name a function after the main thing it does, even if the name doesn't encompass everything it does in all cases, especially in a local context like this where it's not going to be called by faraway code.

    This code definitely sucks and there will be a lot of opportunities to improve it later (see https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking). This commit is just trying to make a simple-as-possible global change to the state representation in order to be able to make local improvements in places like this more easy in the future.

  73. in src/util/overloaded.h:12 in 4e11f88320 outdated
       7 | +
       8 | +#include <optional>
       9 | +#include <utility>
      10 | +
      11 | +//! Overloaded helper for std::visit, useful to write code that switches on a
      12 | +//! variant type and triggers a compile error if there are any unhandled cases.
    


    promag commented at 2:16 PM on April 26, 2021:

    4e11f88320b644b67db55fe737815451ca7d0681

    I've tested this removing one case in TxStateSerializedBlockHash and it failed to compile.


    ryanofsky commented at 3:12 PM on April 30, 2021:

    re: #21206 (review)

    4e11f88

    I've tested this removing one case in TxStateSerializedBlockHash and it failed to compile.

    Thanks for testing!


    maflcko commented at 8:23 AM on July 1, 2021:

    Just for reference, our existing use of std::visit will also throw compile errors even without this helper. I guess the goal of the helper is to having to write less code in the switch itself?


    ryanofsky commented at 9:00 AM on July 1, 2021:

    Just for reference, our existing use of std::visit will also throw compile errors even without this helper. I guess the goal of the helper is to having to write less code in the switch itself?

    I think all existing uses of uses of std::visit use custom visitor classes which don't have access to local variables or anything. Or at least they did at the time this was written which was shortly after we enabled c++17. If it makes sense to define visitor classes for a particular variant type, this isn't really meant to be an alternative to that. This is just meant to be an alternative to using if/else if statements or a switch statement. I can update the comment to make it more clear, unless you're suggesting there might be a better way to use std::visit without this helper. In that case we could get rid of this.


    maflcko commented at 10:21 AM on July 2, 2021:

    Not saying that this way is better, just saying that it will also emit compile errors even without the helper:

    diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
    index 7dc91bebd8..5a717e498c 100644
    --- a/src/wallet/transaction.h
    +++ b/src/wallet/transaction.h
    @@ -8,12 +8,11 @@
     #include <amount.h>
     #include <primitives/transaction.h>
     #include <serialize.h>
    -#include <wallet/ismine.h>
     #include <threadsafety.h>
     #include <tinyformat.h>
    -#include <util/overloaded.h>
     #include <util/strencodings.h>
     #include <util/string.h>
    +#include <wallet/ismine.h>
     
     #include <list>
     #include <variant>
    @@ -85,25 +84,27 @@ static inline TxState TxStateInterpretSerialized(TxStateUnrecognized data)
     //! Get TxState serialized block hash. Inverse of TxStateInterpretSerialized.
     static inline uint256 TxStateSerializedBlockHash(const TxState& state)
     {
    -    return std::visit(Overloaded{
    -        [](const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; },
    -        [](const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; },
    -        [](const TxStateInMempool& in_mempool) { return uint256::ZERO; },
    -        [](const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; },
    -        [](const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; }
    -    }, state);
    +    struct Overloaded {
    +        uint256 operator()(const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; }
    +        uint256 operator()(const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; }
    +        uint256 operator()(const TxStateInMempool& in_mempool) { return uint256::ZERO; }
    +        uint256 operator()(const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; }
    +        uint256 operator()(const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; }
    +    };
    +    return std::visit(Overloaded{}, state);
     }
     
     //! Get TxState serialized block index. Inverse of TxStateInterpretSerialized.
     static inline int TxStateSerializedIndex(const TxState& state)
     {
    -    return std::visit(Overloaded{
    -        [](const TxStateConfirmed& confirmed) { return confirmed.position_in_block; },
    -        [](const TxStateConflicted& conflicted) { return -1; },
    -        [](const TxStateInMempool& in_mempool) { return 0; },
    -        [](const TxStateInactive& inactive) { return inactive.abandoned ? -1 : 0; },
    -        [](const TxStateUnrecognized& unrecognized) { return unrecognized.index; }
    -    }, state);
    +    struct Overloaded {
    +        int operator()(const TxStateConfirmed& confirmed) { return confirmed.position_in_block; }
    +        int operator()(const TxStateConflicted& conflicted) { return -1; }
    +        int operator()(const TxStateInMempool& in_mempool) { return 0; }
    +        int operator()(const TxStateInactive& inactive) { return inactive.abandoned ? -1 : 0; }
    +        int operator()(const TxStateUnrecognized& unrecognized) { return unrecognized.index; }
    +    };
    +    return std::visit(Overloaded{}, state);
     }
     
     
    
  74. in src/wallet/transaction.h:50 in 4e11f88320 outdated
      45 | +//! or be abandoned, never broadcast, or rejected from the mempool for another
      46 | +//! reason.
      47 | +struct TxStateInactive {
      48 | +    bool abandoned;
      49 | +
      50 | +    explicit TxStateInactive(bool abandoned=false) : abandoned(abandoned) {}
    


    promag commented at 2:23 PM on April 26, 2021:

    4e11f88320b644b67db55fe737815451ca7d0681

    nit, space around =.


    ryanofsky commented at 3:40 PM on April 30, 2021:

    re: #21206 (review)

    4e11f88

    nit, space around =.

    Thanks, ran clang-format-diff

  75. in src/wallet/wallet.cpp:999 in 4e11f88320 outdated
     966 |      if (!fill_wtx(wtx, ins.second)) {
     967 |          return false;
     968 |      }
     969 |      // If wallet doesn't have a chain (e.g wallet-tool), don't bother to update txn.
     970 |      if (HaveChain()) {
     971 |          bool active;
    


    promag commented at 2:25 PM on April 26, 2021:

    4e11f88320b644b67db55fe737815451ca7d0681

    Make local in lambda below?


    ryanofsky commented at 3:58 PM on April 30, 2021:

    re: #21206 (review)

    4e11f88

    Make local in lambda below?

    Can do if another change is required. This line isn't changing, and I'm trying to minimize the diff. The variable scope is pretty small already too.

  76. promag commented at 2:56 PM on April 26, 2021: member

    Code review and tested ACK 4e11f88320b644b67db55fe737815451ca7d0681 (including base PR). Some nits for your consideration.

    I think this approach is superior simply because it prevents state inconsistency. Maybe a followup could be to make state members const.

    Another nice touch in this PR is SyncTxState type which makes it clear which states are handled/accepted in SyncTransaction.

  77. DrahtBot cross-referenced this on Apr 27, 2021 from issue wallet: indicate whether a transaction is in the mempool by danben
  78. DrahtBot added the label Needs rebase on Apr 29, 2021
  79. ryanofsky referenced this in commit c9e8a508e5 on Apr 30, 2021
  80. ryanofsky referenced this in commit 4984c4548c on Apr 30, 2021
  81. ryanofsky force-pushed on Apr 30, 2021
  82. ryanofsky commented at 6:15 PM on April 30, 2021: contributor

    Thanks for the review! I really appreciate it on a noisy PR like this where there are so many small changes.


    Rebased 4e11f88320b644b67db55fe737815451ca7d0681 -> a5a537fd95084a2e2912d9625cef41f5f6732efc (pr/txstate.8 -> pr/txstate.9, compare) on top of #21207 pr/txmove.6 due to conflict with #21759, also making some suggested changes

  83. DrahtBot removed the label Needs rebase on Apr 30, 2021
  84. in src/wallet/receive.h:24 in d1582cdc4e outdated
      19 | +CAmount TxGetCredit(const CWallet& wallet, const CTransaction& tx, const isminefilter& filter);
      20 | +
      21 | +bool ScriptIsChange(const CWallet& wallet, const CScript& script) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
      22 | +bool OutputIsChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
      23 | +CAmount OutputGetChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
      24 | +CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx);
    


    S3RK commented at 7:50 AM on May 4, 2021:

    No need to export OutputGetChange and TxGetChange as they are not used outside of receive.cpp


    ryanofsky commented at 10:22 PM on May 20, 2021:

    re: #21206 (review)

    No need to export OutputGetChange and TxGetChange as they are not used outside of receive.cpp

    I think in the future it might be good to move low-level code using IsChange logic inside receive.cpp so IsChange functions could be private. But for GetChange it seems most consistent to try to keeping exposing Get{Credit,Debit,Change} functions as a group. The fact that these two GetChange functions don't have callers I think is more a reflection of poor unit test coverage than that change amounts should be a private part of transaction accounting.

  85. DrahtBot cross-referenced this on May 6, 2021 from issue [Bundle 7/7] validation: Farewell, global Chainstate! by dongcarl
  86. DrahtBot added the label Needs rebase on May 10, 2021
  87. S3RK commented at 6:33 AM on May 19, 2021: contributor

    Not a full review, just a small thing I've noticed looking at #21207

  88. ryanofsky referenced this in commit 264b945fa7 on May 20, 2021
  89. ryanofsky referenced this in commit 0415a53c9c on May 20, 2021
  90. ryanofsky force-pushed on May 20, 2021
  91. DrahtBot removed the label Needs rebase on May 20, 2021
  92. ryanofsky commented at 10:27 PM on May 20, 2021: contributor

    Thanks for checking this!


    Rebased a5a537fd95084a2e2912d9625cef41f5f6732efc -> 3416ade015914ddeb84fad23d2455d974e9f4a74 (pr/txstate.9 -> pr/txstate.10, compare) on top of #21207 pr/txmove.7 due to conflicts Rebased 3416ade015914ddeb84fad23d2455d974e9f4a74 -> 7cc2549759c12182063fce56161ce4403502312e (pr/txstate.10 -> pr/txstate.11, compare) on top of #21207 pr/txmove.8 due to conflicts with #17331 Rebased 7cc2549759c12182063fce56161ce4403502312e -> c2b3b4b184148447b695ff6ec56bbc2c24447655 (pr/txstate.11 -> pr/txstate.12, compare) on top of #22100 pr/txfun.1 after #21207 merge Rebased c2b3b4b184148447b695ff6ec56bbc2c24447655 -> c00b540d3e315d52f171678e3e6d5e36211d3c38 (pr/txstate.12 -> pr/txstate.13, compare) on top of #22100 pr/txfun.2 Rebased c00b540d3e315d52f171678e3e6d5e36211d3c38 -> b71a25b492d8ded339351ed770a4360ec2316b6c (pr/txstate.13 -> pr/txstate.14, compare) on top of #22100 pr/txfun.3 due to conflict with #21866 Rebased b71a25b492d8ded339351ed770a4360ec2316b6c -> 730df28e00822f06c9fc11c86a74f764a2f5136c (pr/txstate.14 -> pr/txstate.15, compare) on top of #22100 pr/txfun.4

  93. DrahtBot cross-referenced this on May 21, 2021 from issue wallet: Decide which coin selection solution to use based on waste metric by achow101
  94. DrahtBot cross-referenced this on May 21, 2021 from issue wallet: Cleanup and refactor CreateTransactionInternal by achow101
  95. DrahtBot cross-referenced this on May 21, 2021 from issue refactor: Switch serialize to uint8_t (Bundle 1/2) by maflcko
  96. DrahtBot cross-referenced this on May 21, 2021 from issue Wallet passive startup by ryanofsky
  97. DrahtBot cross-referenced this on May 22, 2021 from issue wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101
  98. DrahtBot cross-referenced this on May 24, 2021 from issue Replace size/weight estimate tuple with struct for named fields by instagibbs
  99. DrahtBot added the label Needs rebase on May 25, 2021
  100. ryanofsky referenced this in commit ed565f93e2 on May 25, 2021
  101. ryanofsky referenced this in commit ae93b95a68 on May 25, 2021
  102. ryanofsky force-pushed on May 25, 2021
  103. DrahtBot removed the label Needs rebase on May 25, 2021
  104. DrahtBot added the label Needs rebase on May 26, 2021
  105. ryanofsky referenced this in commit c7bd5842e4 on May 26, 2021
  106. ryanofsky referenced this in commit 9ece3479c9 on May 26, 2021
  107. meshcollider referenced this in commit 55a156fca0 on May 30, 2021
  108. ryanofsky force-pushed on May 30, 2021
  109. DrahtBot removed the label Needs rebase on May 30, 2021
  110. ryanofsky cross-referenced this on May 30, 2021 from issue refactor: Clean up new wallet spend, receive files added #21207 by ryanofsky
  111. DrahtBot cross-referenced this on Jun 5, 2021 from issue wallet test: Add test for subtract fee from recipient behavior by ryanofsky
  112. DrahtBot added the label Needs rebase on Jun 9, 2021
  113. ryanofsky force-pushed on Jun 11, 2021
  114. DrahtBot removed the label Needs rebase on Jun 11, 2021
  115. DrahtBot added the label Needs rebase on Jun 12, 2021
  116. ryanofsky force-pushed on Jun 14, 2021
  117. DrahtBot removed the label Needs rebase on Jun 14, 2021
  118. DrahtBot cross-referenced this on Jun 15, 2021 from issue Enable external signer support by default, reduce #ifdef by Sjors
  119. DrahtBot added the label Needs rebase on Jun 17, 2021
  120. ryanofsky force-pushed on Jun 17, 2021
  121. DrahtBot removed the label Needs rebase on Jun 18, 2021
  122. rebroad referenced this in commit 7c229d7a74 on Jun 23, 2021
  123. DrahtBot cross-referenced this on Jun 28, 2021 from issue wallet: Do not set fInMempool in transactionAddedToMempool when tx is not in the mempool by maflcko
  124. in src/wallet/transaction.h:86 in 730df28e00 outdated
      82 | +    }
      83 | +    return data;
      84 | +}
      85 | +
      86 | +//! Get TxState serialized block hash. Inverse of TxStateInterpretSerialized.
      87 | +static uint256 TxStateSerializedBlockHash(const TxState& state)
    


    jonatack commented at 2:04 PM on June 28, 2021:

    730df28e0082 seeing the following warnings when debug-building with debian clang 11

    ./wallet/transaction.h:221:59: warning: field 'tx' will be initialized after field 'm_state' [-Wreorder-ctor]
        CWalletTx(CTransactionRef tx, const TxState& state) : tx(std::move(tx)), m_state(state)
                                                              ^
    ./wallet/transaction.h:71:16: warning: 'static' function 'TxStateInterpretSerialized' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
    static TxState TxStateInterpretSerialized(TxStateUnrecognized data)
                   ^
    ./wallet/transaction.h:86:16: warning: 'static' function 'TxStateSerializedBlockHash' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
    static uint256 TxStateSerializedBlockHash(const TxState& state)
                   ^
    ./wallet/transaction.h:98:12: warning: 'static' function 'TxStateSerializedIndex' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
    static int TxStateSerializedIndex(const TxState& state)
               ^
    4 warnings generated.
    

    ryanofsky commented at 10:10 PM on July 1, 2021:

    730df28 seeing the following warnings when debug-building with debian clang 11

    Thanks! Should be fixed in new push

  125. DrahtBot cross-referenced this on Jun 30, 2021 from issue Support multiple -*notify commands by luke-jr
  126. ryanofsky force-pushed on Jul 1, 2021
  127. ryanofsky force-pushed on Jul 1, 2021
  128. ryanofsky force-pushed on Jul 1, 2021
  129. ryanofsky commented at 10:13 PM on July 1, 2021: contributor

    Updated 730df28e00822f06c9fc11c86a74f764a2f5136c -> c5ee3fb0b1db4095376bcff074741dc7a135ba8a (pr/txstate.15 -> pr/txstate.16, compare) fixing reported compiler warnings and improving std::visit comment Rebased c5ee3fb0b1db4095376bcff074741dc7a135ba8a -> 8f25d3a6b7a6cc308eb2f0ebc944e1dd425902f5 (pr/txstate.16 -> pr/txstate.17, compare) due to conflict with #22492 Rebased 8f25d3a6b7a6cc308eb2f0ebc944e1dd425902f5 -> c5599e514beb28518e4f31f495016cf6d8cd4c24 (pr/txstate.17 -> pr/txstate.18, compare) due to conflict with #22359 Rebased c5599e514beb28518e4f31f495016cf6d8cd4c24 -> 095b1d6f58e2302f229617a2439e5a57e14fe936 (pr/txstate.18 -> pr/txstate.19, compare) on top of pr/txfun.6 #22100 Rebased 095b1d6f58e2302f229617a2439e5a57e14fe936 -> b17ba1c752a65d2f8b3d1485074d08d48c1ffd3e (pr/txstate.19 -> pr/txstate.20, compare) on top of pr/txfun.7 #22100

  130. DrahtBot cross-referenced this on Jul 19, 2021 from issue wallet: Reorder locks in dumpwallet to avoid lock order assertion by achow101
  131. DrahtBot added the label Needs rebase on Jul 20, 2021
  132. ryanofsky force-pushed on Jul 22, 2021
  133. DrahtBot removed the label Needs rebase on Jul 22, 2021
  134. DrahtBot added the label Needs rebase on Jul 27, 2021
  135. ryanofsky force-pushed on Aug 13, 2021
  136. DrahtBot removed the label Needs rebase on Aug 13, 2021
  137. DrahtBot added the label Needs rebase on Aug 19, 2021
  138. ryanofsky force-pushed on Aug 19, 2021
  139. DrahtBot removed the label Needs rebase on Aug 19, 2021
  140. DrahtBot cross-referenced this on Aug 26, 2021 from issue refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack
  141. DrahtBot added the label Needs rebase on Sep 1, 2021
  142. meshcollider referenced this in commit 629c4ab2e3 on Sep 3, 2021
  143. promag commented at 9:28 AM on September 3, 2021: member

    Base PR is merged, could update OP and rebase to move this change forward.

  144. ryanofsky force-pushed on Sep 3, 2021
  145. ryanofsky commented at 6:34 PM on September 3, 2021: contributor

    Base PR is merged, could update OP and rebase to move this change forward.

    Thanks, rebased and removed references to base PR in the OP.

    Rebased b17ba1c752a65d2f8b3d1485074d08d48c1ffd3e -> 2896ad570523842245d2de77de531776cfcc08d8 (pr/txstate.20 -> pr/txstate.21, compare) after base PR #22100 merge. Rebased 2896ad570523842245d2de77de531776cfcc08d8 -> 541d5f5c775a9aa884c1a8b37fc123933fdbd98e (pr/txstate.21 -> pr/txstate.22, compare) due to conflict with #20591

  146. DrahtBot removed the label Needs rebase on Sep 3, 2021
  147. DrahtBot cross-referenced this on Sep 15, 2021 from issue doc: Fix incorrect C++ named args by maflcko
  148. DrahtBot added the label Needs rebase on Sep 29, 2021
  149. ryanofsky force-pushed on Oct 2, 2021
  150. DrahtBot removed the label Needs rebase on Oct 2, 2021
  151. meshcollider commented at 12:56 AM on October 4, 2021: contributor

    Code review ACK 541d5f5c775a9aa884c1a8b37fc123933fdbd98e

    Looks great!

  152. DrahtBot cross-referenced this on Oct 15, 2021 from issue tests: remove usage of LegacyScriptPubKeyMan from some wallet tests by achow101
  153. DrahtBot cross-referenced this on Oct 22, 2021 from issue doc: Fix CWalletTx::Confirmation doc by maflcko
  154. DrahtBot added the label Needs rebase on Oct 22, 2021
  155. ryanofsky force-pushed on Oct 25, 2021
  156. ryanofsky commented at 4:18 PM on October 25, 2021: contributor

    Rebased 541d5f5c775a9aa884c1a8b37fc123933fdbd98e -> bb16e10149c4e1bf2e3d0ee027e9025cd48dd65f (pr/txstate.22 -> pr/txstate.23, compare) due to conflicts with #23288 and #23338 Rebased bb16e10149c4e1bf2e3d0ee027e9025cd48dd65f -> 3c6fdae6ed67426bd9ddb2949a9e66934f71bf0b (pr/txstate.23 -> pr/txstate.24, compare) due to conflict with #23332

  157. DrahtBot removed the label Needs rebase on Oct 25, 2021
  158. DrahtBot cross-referenced this on Oct 25, 2021 from issue refactor: Remove `gArgs` from `wallet.h` and `wallet.cpp` (2) by kiminuo
  159. DrahtBot added the label Needs rebase on Oct 26, 2021
  160. janus referenced this in commit 6856f8c5c2 on Oct 29, 2021
  161. ryanofsky force-pushed on Oct 29, 2021
  162. DrahtBot removed the label Needs rebase on Oct 29, 2021
  163. DrahtBot cross-referenced this on Nov 2, 2021 from issue refactor: Avoid integer overflow in ApplyStats when activating snapshot by maflcko
  164. DrahtBot cross-referenced this on Nov 3, 2021 from issue Fix signed integer overflow in prioritisetransaction RPC by maflcko
  165. DrahtBot added the label Needs rebase on Nov 10, 2021
  166. refactor: Make CWalletTx sync state type-safe
    Current CWalletTx state representation makes it possible to set
    inconsistent states that won't be handled correctly by wallet sync code
    or serialized & deserialized back into the same form.
    
    For example, it is possible to call setConflicted without setting a
    conflicting block hash, or setConfirmed with no transaction index. And
    it's possible update individual m_confirm and fInMempool data fields
    without setting an overall consistent state that can be serialized and
    handled correctly.
    
    Fix this without changing behavior by using std::variant, instead of an
    enum and collection of fields, to represent sync state, so state
    tracking code is safer and more legible.
    
    This is a first step to fixing state tracking bugs
    https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
    by adding an extra margin of safety that can prevent new bugs from being
    introduced as existing bugs are fixed.
    d8ee8f3cd3
  167. ryanofsky force-pushed on Nov 15, 2021
  168. ryanofsky commented at 3:15 PM on November 15, 2021: contributor

    Rebased 3c6fdae6ed67426bd9ddb2949a9e66934f71bf0b -> 78c8ac87e2f585772f296b943c733bd26f255ba0 (pr/txstate.24 -> pr/txstate.25, compare) due to conflict with #22928, also making minor MakeWalletTxStatus cleanup

  169. DrahtBot removed the label Needs rebase on Nov 15, 2021
  170. maflcko added this to the "Blockers" column in a project

  171. DrahtBot cross-referenced this on Nov 16, 2021 from issue Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces by ryanofsky
  172. ryanofsky cross-referenced this on Nov 18, 2021 from issue Future PRs by jnewbery
  173. DrahtBot cross-referenced this on Nov 18, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments by maflcko
  174. meshcollider commented at 1:42 AM on November 22, 2021: contributor

    re-utACK 78c8ac87e2f585772f296b943c733bd26f255ba0

  175. maflcko requested review from promag on Nov 22, 2021
  176. maflcko requested review from jonatack on Nov 22, 2021
  177. jonatack commented at 2:16 PM on November 22, 2021: contributor

    Concept ACK, wil review today.

  178. in src/wallet/rpcdump.cpp:374 in 78c8ac87e2 outdated
     368 | @@ -369,11 +369,9 @@ RPCHelpMan importprunedfunds()
     369 |  
     370 |      unsigned int txnIndex = vIndex[it - vMatch.begin()];
     371 |  
     372 | -    CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, merkleBlock.header.GetHash(), txnIndex);
     373 | -
     374 |      CTransactionRef tx_ref = MakeTransactionRef(tx);
     375 |      if (pwallet->IsMine(*tx_ref)) {
     376 | -        pwallet->AddToWallet(std::move(tx_ref), confirm);
     377 | +        pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, (int)txnIndex});
    


    jonatack commented at 6:33 PM on November 22, 2021:

    if you retouch, per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named (unless there is a reason to not follow the guideline)

            pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, static_cast<int>(txnIndex)});
    

    (this is also the change suggested by clang)

    wallet/rpcdump.cpp:374:104: error: non-constant-expression cannot be narrowed from type 'unsigned int' to 'int' in initializer list [-Wc++11-narrowing]
            pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, txnIndex});
                                                                                                           ^~~~~~~~
    wallet/rpcdump.cpp:374:104: note: insert an explicit cast to silence this issue
            pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, txnIndex});
                                                                                                           ^~~~~~~~
                                                                                                           static_cast<int>( )
    1 error generated.
    

    ryanofsky commented at 11:51 PM on November 23, 2021:

    re: #21206 (review)

    Ok. I don't see how this helps readability or could help catching errors here, but I guess I'll give benefit of the doubt and change.

    wallet/rpcdump.cpp:374:104: error: non-constant-expression cannot be narrowed from type 'unsigned int' to 'int' in initializer list [-Wc++11-narrowing]
    

    This warning doesn't happen with either cast, so it doesn't seem like a good reason to chose the more verbose cast.

    I think when you have a specific point to make, it's good to cite an external guideline to help make the point. But I don't think it's good to just parrot a guideline without saying what problem it's addressing in the relevant context.


    jonatack commented at 11:45 AM on November 24, 2021:

    Yes, the developer-notes mention to follow the CppCoreGuidelines and there are some advantages to named casts (https://github.com/bitcoin/bitcoin/pull/20965#issuecomment-770375623) but I do hesitate to mention them due to the verbosity (though if casts are seen as a code smell, verbosity could be a good thing, I guess). Thanks!

  179. in src/wallet/wallet.h:579 in 78c8ac87e2 outdated
     575 | @@ -576,7 +576,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     576 |      void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm);
     577 |  
     578 |      /** Pass this transaction to node for mempool insertion and relay to peers if flag set to true */
     579 | -    bool SubmitTxMemoryPoolAndRelay(const CWalletTx& wtx, std::string& err_string, bool relay) const;
     580 | +    bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const;
    


    jonatack commented at 6:55 PM on November 22, 2021:

    Perhaps update the Doxygen documentation in the previous line to explain why wtx is after this change mutable / an out-param.


    ryanofsky commented at 1:06 AM on November 24, 2021:

    re: #21206 (review)

    Perhaps update the Doxygen documentation in the previous line to explain why wtx is after this change mutable / an out-param.

    I'll add your text if you want to make a specific suggestion, but mentioning the wtx state update seems like an unusual detail for this summary comment to focus on when it doesn't going into other details like this, and there are already lots of comments about refreshing the in-mempool state. Just changing this to a mutable reference already seems more straightforward than the previous version which used a const reference and mutable state member.

  180. in src/wallet/wallet.cpp:1644 in 78c8ac87e2 outdated
    1640 | @@ -1645,7 +1641,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
    1641 |                  break;
    1642 |              }
    1643 |              for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
    1644 | -                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate, /* rescanning_old_block */ true);
    1645 | +                SyncTransaction(block.vtx[posInBlock], TxStateConfirmed{block_hash, block_height, (int)posInBlock}, fUpdate, /* rescanning_old_block */ true);
    


    jonatack commented at 7:04 PM on November 22, 2021:

    Here and elsewhere in this commit, maybe format the named args to be compatible with clang-tidy in a way consistent with current changes in the codebase (see #23546 and #23545).

                    SyncTransaction(block.vtx[posInBlock], TxStateConfirmed{block_hash, block_height, static_cast<int>(posInBlock)}, fUpdate, /*rescanning_old_block=*/true);
    

    also, use a named cast for posInBlock


    ryanofsky commented at 12:35 AM on November 24, 2021:

    re: #21206 (review)

    Here and elsewhere in this commit, maybe format the named args to be compatible with clang-tidy in a way consistent with current changes in the codebase (see #23546 and #23545).

    also, use a named cast for posInBlock

    Ok, made these changes

  181. laanwj commented at 2:01 PM on November 23, 2021: member

    I like this refactor, it abstracts the transaction state much better than the boolean-radio-buttons, and agree it makes some kinds of mistake harder to make.

    Concept and code review ACK 78c8ac87e2f585772f296b943c733bd26f255ba0 re-ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f

  182. in src/wallet/transaction.h:71 in 78c8ac87e2 outdated
      66 | +
      67 | +//! Subset of states transaction sync logic is implemented to handle.
      68 | +using SyncTxState = std::variant<TxStateConfirmed, TxStateInMempool, TxStateInactive>;
      69 | +
      70 | +//! Interpret TxState serialized fields as a recognized state.
      71 | +static inline TxState TxStateInterpretSerialized(TxStateUnrecognized data)
    


    jonatack commented at 6:09 PM on November 23, 2021:

    Seems odd to pass a TxStateUnrecognized data type for a recognized state. Edit: maybe suggest the fields are "converted from an unrecognized to recognized state."


    ryanofsky commented at 12:05 AM on November 24, 2021:

    re: #21206 (review)

    Seems odd to pass a TxStateUnrecognized data type for a recognized state. Edit: maybe suggest the fields are "converted from an unrecognized to recognized state."

    Thanks, rephrased now.

    Just to explain the previous version: "Interpret TxState serialized fields as a recognized state" was supposed to be read like "Interpret serialized bytes as an integer"

  183. in src/wallet/transaction.h:93 in 78c8ac87e2 outdated
      88 | +    return std::visit(Overloaded{
      89 | +        [](const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; },
      90 | +        [](const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; },
      91 | +        [](const TxStateInMempool& in_mempool) { return uint256::ZERO; },
      92 | +        [](const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; },
      93 | +        [](const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; }
    


    jonatack commented at 6:11 PM on November 23, 2021:

    nit, perhaps order the state methods above in the order used here and in the next function, or vice versa.


    ryanofsky commented at 12:09 AM on November 24, 2021:

    re: #21206 (review)

    nit, perhaps order the state methods above in the order used here and in the next function, or vice versa.

    Good suggestion. Fixed!

    (Assuming you meant "previous function" instead of "next function" since next function already used the same order)

  184. promag commented at 8:48 PM on November 23, 2021: member

    Code review ACK 78c8ac87e2f585772f296b943c733bd26f255ba0.

    In a follow-up, we can consolidate updates to m_state, for instance, the 2nd update can be avoided:

    transactionAddedToMempool
        SyncTransaction
            AddToWalletIfInvolvingMe
                AddToWallet
                    m_state = ...
        RefreshMempoolStatus
            m_state = ...
    
  185. in src/wallet/transaction.h:132 in 78c8ac87e2 outdated
     210 | @@ -129,44 +211,12 @@ class CWalletTx
     211 |          nTimeSmart = 0;
     212 |          fFromMe = false;
     213 |          fChangeCached = false;
     214 | -        fInMempool = false;
    


    jonatack commented at 9:19 PM on November 23, 2021:

    There appear to be remaining fInMempool comments that may need updating.

    src/wallet/wallet.cpp:1733:    // We must set fInMempool here - while it will be re-set to true by the
    src/wallet/wallet.cpp-1734-    // entered-mempool callback, if we did not there would be a race where a
    src/wallet/wallet.cpp-1735-    // user could call sendmoney in a loop and hit spurious out of funds errors
    src/wallet/wallet.cpp-1736-    // because we think that this newly generated transaction's change is
    src/wallet/wallet.cpp-1737-    // unavailable as we're not yet aware that it is in the mempool.
    src/wallet/wallet.cpp-1738-    //
    src/wallet/wallet.cpp:1739:    // Irrespective of the failure reason, un-marking fInMempool
    src/wallet/wallet.cpp-1740-    // out-of-order is incorrect - it should be unmarked when
    src/wallet/wallet.cpp-1741-    // TransactionRemovedFromMempool fires.
    --
    src/wallet/wallet.cpp:1973    // Get the inserted-CWalletTx from mapWallet so that the
    src/wallet/wallet.cpp:1974    // fInMempool flag is cached properly
    

    ryanofsky commented at 12:12 AM on November 24, 2021:

    re: #21206 (review)

    There appear to be remaining fInMempool comments that may need updating.

    Nice catch! Updated these

  186. in src/util/overloaded.h:9 in 78c8ac87e2 outdated
       0 | @@ -0,0 +1,23 @@
       1 | +// Copyright (c) 2021 The Bitcoin Core developers
       2 | +// Distributed under the MIT software license, see the accompanying
       3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
       4 | +
       5 | +#ifndef BITCOIN_UTIL_OVERLOADED_H
       6 | +#define BITCOIN_UTIL_OVERLOADED_H
       7 | +
       8 | +#include <optional>
       9 | +#include <utility>
    


    jonatack commented at 9:22 PM on November 23, 2021:

    Should these two includes be replaced by #include <variant>?


    ryanofsky commented at 11:34 PM on November 23, 2021:

    re: #21206 (review)

    Should these two includes be replaced by #include <variant>?

    Good catch. I think these were just copied and pasted from another header. The variant type is not actually referenced so I believe no includes are necessary.

  187. jonatack commented at 9:26 PM on November 23, 2021: contributor

    Approach ACK / light code review ACK 78c8ac87e2f585772f296b943c733bd26f255ba0

  188. in src/wallet/wallet.cpp:1000 in 78c8ac87e2 outdated
    1002 | -        if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) {
    1003 | -            // Update cached block height variable since it not stored in the
    1004 | -            // serialized transaction.
    1005 | -            wtx.m_confirm.block_height = height;
    1006 | -        } else if (wtx.isConflicted() || wtx.isConfirmed()) {
    1007 | +        auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
    


    jonatack commented at 9:33 PM on November 23, 2021:
            auto lookup_block = [&](const uint256& hash, int height, TxState& state) {
    

    ryanofsky commented at 12:34 AM on November 24, 2021:

    re: #21206 (review)

    Sorry, this is an output variable, so making this change would leave heights unset. I want to scrap this code completely after this PR (see previous review comments), but the goal for this PR is to make the smallest possible code changes in the wallet everywhere that switch to a new CWalletTx state representation without changing behavior. Then future PRs can clean up the code and nonsensical behavior in individual places like this.


    jonatack commented at 11:23 AM on November 24, 2021:

    Oh ok, thanks. I didn't see that findBlock() uses height as an out-param, but now had a better look at the FoundBlock class.

  189. ryanofsky force-pushed on Nov 24, 2021
  190. ryanofsky commented at 1:26 AM on November 24, 2021: contributor

    Thanks for reviews and suggestions!

    Updated 78c8ac87e2f585772f296b943c733bd26f255ba0 -> d8ee8f3cd32bbfefec931724f5798cbb088ceb6f (pr/txstate.25 -> pr/txstate.26, compare)

  191. in src/wallet/test/wallet_tests.cpp:369 in d8ee8f3cd3
     370 | -
     371 | -    // If transaction is already in map, to avoid inconsistencies, unconfirmation
     372 | -    // is needed before confirm again with different block.
     373 | -    return wallet.AddToWallet(MakeTransactionRef(tx), confirm, [&](CWalletTx& wtx, bool /* new_tx */) {
     374 | -        wtx.setUnconfirmed();
     375 | +    return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, bool /* new_tx */) {
    


    jonatack commented at 11:03 AM on November 24, 2021:

    (if need to retouch)

        return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, /*new_tx=*/bool) {
    
  192. in src/wallet/wallet.cpp:1003 in d8ee8f3cd3
    1005 | -            wtx.m_confirm.block_height = height;
    1006 | -        } else if (wtx.isConflicted() || wtx.isConfirmed()) {
    1007 | +        auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
    1008 |              // If tx block (or conflicting block) was reorged out of chain
    1009 |              // while the wallet was shutdown, change tx status to UNCONFIRMED
    1010 |              // and reset block height, hash, and index. ABANDONED tx don't have
    


    jonatack commented at 11:19 AM on November 24, 2021:

    Should the documentation be updated here as well?

  193. jonatack commented at 11:48 AM on November 24, 2021: contributor

    Code review ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f

  194. laanwj removed this from the "Blockers" column in a project

  195. laanwj merged this on Nov 25, 2021
  196. laanwj closed this on Nov 25, 2021

  197. sidhujag referenced this in commit ebd73c7089 on Nov 26, 2021
  198. bitcoin locked this on Apr 11, 2023
  199. in src/util/overloaded.h:18 in d8ee8f3cd3
      13 | +//!
      14 | +//! Implementation comes from and example usage can be found at
      15 | +//! https://en.cppreference.com/w/cpp/utility/variant/visit#Example
      16 | +template<class... Ts> struct Overloaded : Ts... { using Ts::operator()...; };
      17 | +
      18 | +//! Explicit deduction guide (not needed as of C++20)
    


    hebasto commented at 9:16 PM on January 4, 2024:

    As for Clang, this explicit deduction guide can be dropped for versions >=17 only, unfortunately.


    fanquake commented at 10:03 AM on January 5, 2024:

    This is part of #29042.


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-05-20 06:54 UTC