RFC: consensus: Make `CAmount` a class #35511

pull hodlinator wants to merge 15 commits into bitcoin:master from hodlinator:2026/06/safer_camount.take2 changing 176 files +1466 −1256
  1. hodlinator commented at 10:06 AM on June 11, 2026: contributor

    We should treat people's money with care across node and wallet.

    Having our CAmount be a typedef for raw int64_t as on master has the following drawbacks:

    • Allows assigning to from bools, leading to the issues like the one previously fixed by 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab.
    • Allows direct comparison with integral types of differing size, leading to things like the issue previously fixed by 890a09b1e49925315a5636f29cc5af36928fe092 (reverting it on top of this PR leads to compile errors).
    • Allows forgetting to initialize amounts (CAmount fee;).
    • Allows direct operations with decimal types as in CAmount target = 49.5L * COIN in src/wallet/test/coinselector_tests.cpp.
    • Allows multiplying two CAmounts with each other, which does not make sense.

    Beyond fixing the above, this PR also makes amounts stick out more in code - either by the CAmount type itself or by using the _sats user defined literal.

    This work is also a stepping stone for possible future work introducing an even safer amount type which only allows values in the range [0, MAX_MONEY].

    Already merged PRs spawned out of this work: #35456, #35372

    Prior attempts in similar directions: #5430, #4067

  2. DrahtBot commented at 10:06 AM on June 11, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35511.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK sedited, ryanofsky

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #bitcoin-core/gui/911 (Adds non-mempool wallet balance to overview by ajtowns)
    • #35522 (refactor: Extract per-message helpers from SendMessages() (move-only) by pablomartin4btc)
    • #35501 (wallet: store all witness variants of a transaction by achow101)
    • #35295 (validation: fetch block input prevouts in parallel during ConnectBlock by andrewtoth)
    • #35105 (Refactor: Updated TransactionError to use util::Expected and removed TransactionError:OK by kevkevinpal)
    • #35084 (ipc: Support for windows support by ryanofsky)
    • #34872 (wallet: fix mixed-input transaction accounting in history RPCs by w0xlt)
    • #34824 (net: encapsulate TxRelay state and replace recursive mutexes by w0xlt)
    • #34698 (wallet: handle MiniMiner bump fee calculation failures by shuv-amp)
    • #34617 (fees: wallet: remove block policy fee estimator internals from wallet by ismaelsadeeq)
    • #34514 (refactor: remove unnecessary std::move for a few trivially copyable types by l0rinc)
    • #34405 (wallet: skip APS when no partial spend exists by 8144225309)
    • #34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
    • #34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)
    • #33741 (rpc: Optionally print feerates in sat/vb by polespinasa)
    • #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
    • #29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
    • #29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • everytime adding a transaction -> every time adding a transaction [“everytime” is misspelled]

    Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • BitcoinUnits::formatWithUnit(unit, CAmount{amount}, true, BitcoinUnits::SeparatorStyle::ALWAYS) in src/qt/overviewpage.cpp
    • changeset->StageAddition(tx1_conflict, tx1_fee+1_sats, 0, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
    • changeset->StageAddition(tx1_conflict, tx1_fee, 0, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
    • changeset->StageAddition(tx3, tx2_fee, 0, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
    • changeset->StageAddition(entry4.GetSharedTx(), tx2_fee + entry5->GetModifiedFee() + 1_sats, 0, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp

    Possible places where comparison-specific test macros should replace generic comparisons:

    • [src/test/rpc_tests.cpp] BOOST_CHECK_THROW(AmountFromValue(ValueFromString("-0.00000001")), UniValue); -> Consider BOOST_CHECK_EXCEPTION(..., UniValue, /*predicate on the expected error details*/) so the test verifies the specific failure reason instead of only the thrown type.

    <sup>2026-06-18 20:36:04</sup>

  3. sedited commented at 10:26 AM on June 11, 2026: contributor

    Concept ACK

  4. DrahtBot added the label CI failed on Jun 11, 2026
  5. DrahtBot commented at 11:40 AM on June 11, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/27339510417/job/80774458061</sub> <sub>LLM reason (✨ experimental): CI failed because the fuzz target wouldn’t compile: clang++ reported -Wc++11-narrowing errors in coinselection.cpp when initializing CAmount from group_pos.size().</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  6. in src/ipc/capnp/common-types.h:137 in c14d67a309
     132 | +void CustomBuildField(TypeList<CAmount>, Priority<1>, InvokeContext& invoke_context, Value&& value, Output&& output)
     133 | +{
     134 | +    const CAmount::inner_type& source{value.Int()};
     135 | +    CAmount::inner_type dest{output.get()};
     136 | +    // Assumes both ends are using the same byte order.
     137 | +    memcpy(&dest, &source, sizeof(source));
    


    janb84 commented at 1:17 PM on June 11, 2026:
        output.set(value.Int());
        // OR output.set(static_cast<CAmount::inner_type>(value.Int()));
    

    Current code gets a local copy, overrides the local copy and local copy gets destroyed at the end of the function. Nothing is written back to the Cap'n Proto message.

    <details>

    Regression test:

    diff --git a/src/ipc/test/ipc_test.capnp b/src/ipc/test/ipc_test.capnp
    @@ -15,6 +15,7 @@ using Mining = import "../capnp/mining.capnp";
     
     interface FooInterface $Proxy.wrap("FooImplementation") {
         add [@0](/bitcoin-bitcoin/contributor/0/) (a :Int32, b :Int32) -> (result :Int32);
    +    passAmounts [@6](/bitcoin-bitcoin/contributor/6/) (arg :List(Int64)) -> (result :List(Int64));
         passOutPoint [@1](/bitcoin-bitcoin/contributor/1/) (arg :Data) -> (result :Data);
         passUniValue [@2](/bitcoin-bitcoin/contributor/2/) (arg :Text) -> (result :Text);
         passTransaction [@3](/bitcoin-bitcoin/contributor/3/) (arg :Data) -> (result :Data);
    
    diff --git a/src/ipc/test/ipc_test.h b/src/ipc/test/ipc_test.h
    @@ -15,6 +15,7 @@ class FooImplementation
     {
     public:
         int add(int a, int b) { return a + b; }
    +    std::vector<CAmount> passAmounts(std::vector<CAmount> v) { return v; }
         COutPoint passOutPoint(COutPoint o) { return o; }
         UniValue passUniValue(UniValue v) { return v; }
         CTransactionRef passTransaction(CTransactionRef t) { return t; }
    
    diff --git a/src/ipc/test/ipc_test.cpp b/src/ipc/test/ipc_test.cpp
    @@ -84,6 +84,15 @@ void IpcPipeTest()
         // Test: make sure arguments were sent and return value is received
         BOOST_CHECK_EQUAL(foo->add(1, 2), 3);
     
    +    // Test for CustomBuildField / CustomReadField overloads for 
    +    //  CAmount in ipc/capnp/common-types.h. Each value is serialized
    +    //  once as the argument and once as the return value, so
    +    // a build field that fails to write its value shows up as a zeroed result.
    +    const std::vector<CAmount> amounts1{0_sats, 1_sats, COIN, MAX_MONEY, CAmount{-1}};
    +    const std::vector<CAmount> amounts2{foo->passAmounts(amounts1)};
    +    BOOST_CHECK_EQUAL_COLLECTIONS(amounts1.begin(), amounts1.end(), amounts2.begin(), amounts2.end());
    +
         COutPoint txout1{Txid::FromUint256(uint256{100}), 200};
         COutPoint txout2{foo->passOutPoint(txout1)};
         BOOST_CHECK(txout1 == txout2);
    

    </details>


    hodlinator commented at 6:16 PM on June 11, 2026:

    Thanks for catching this! I'm surprised it was not covered by tests earlier. I guess transaction serialization has the CAmounts outputs serialized as part of a byte-stream before reaching our Cap'n Proto integration layer. Added your test in a separate commit (44e5ba33d8ded7c1d72ce29321bcf839aacfb7e4) in the latest push (6b8a616b60541c77120525f74ebb11e422373a44), omitting the comment (let me know if you disagree, I think it was good context for your comment here in the PR but a bit verbose in the context of the test).


    janb84 commented at 8:31 AM on June 12, 2026:

    yeah test was for proving the bug not the intention to add it so i'm glad you altered it somewhat !

  7. in src/node/mempool_persist.cpp:99 in c14d67a309
      95 | @@ -96,8 +96,8 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
      96 |                  nTime = TicksSinceEpoch<std::chrono::seconds>(now);
      97 |              }
      98 |  
      99 | -            CAmount amountdelta = nFeeDelta;
     100 | -            if (amountdelta && opts.apply_fee_delta_priority) {
     101 | +            CAmount amountdelta{nFeeDelta};
    


    janb84 commented at 3:57 PM on June 11, 2026:
    
    

    NIT: Change suggestion isn't helpful but now that we have new Serialization/deserialization, deserializes straight into a CAmount amountdelta. Dropping the need for the int64_t nFeeDelta temporary and the separate CAmount amountdelta{nFeeDelta} wrap.

    
    CAmount amountdelta{0};
    file >> TX_WITH_WITNESS(tx);
    file >> nTime;
    file >> amountdelta;
    
    if (opts.use_current_time) {
    nTime = TicksSinceEpoch<std::chrono::seconds>(now);
    }
    
    

    hodlinator commented at 6:19 PM on June 11, 2026:

    Agreed - switched in latest push (6b8a616b60541c77120525f74ebb11e422373a44) to serializing and checking CAmount fee_delta{0};. I think nFeeDelta was a more descriptive name, but altered it to match current naming style.

  8. hodlinator force-pushed on Jun 11, 2026
  9. DrahtBot removed the label CI failed on Jun 11, 2026
  10. ryanofsky commented at 9:30 PM on June 15, 2026: contributor

    Concept ACK. New CAmount implementation seems simple and clean and adds safety benefits.

    I haven't looked through the commits yet, but the PR seems very big. Could it make sense to split up somehow, maybe by introducing a new amount class and beginning to use that in places where it's most important, and gradually replacing CAmount, instead of replacing it everywhere? Or maybe there are other ways to split it up? Or maybe it is trivial to review and splitting it up would not be helpful? Curious what you think. This is just larger than I might have expected.

  11. in src/consensus/amount.h:113 in 6b8a616b60
     109 | +    constexpr const inner_type& Int() const noexcept { return m_sats; }
     110 | +};
     111 | +
     112 | +consteval CAmount operator""_sats(unsigned long long amount) noexcept
     113 | +{
     114 | +    assert(amount <= decltype(amount){std::numeric_limits<CAmount::inner_type>::max()});
    


    maflcko commented at 6:27 AM on June 16, 2026:

    q: Shouldn't this check against MAX_MONEY?

  12. in src/consensus/amount.h:108 in 6b8a616b60
     104 | +    {
     105 | +        *this = CAmount{m_sats >> other};
     106 | +        return *this;
     107 | +    }
     108 | +
     109 | +    constexpr const inner_type& Int() const noexcept { return m_sats; }
    


    maflcko commented at 6:27 AM on June 16, 2026:

    Either mark with LIFETIMEBOUND, or just return a copy, which is usually not slower anyway?

  13. maflcko commented at 6:33 AM on June 16, 2026: member

    I haven't looked through the commits yet, but the PR seems very big. Could it make sense to split up somehow, maybe by introducing a new amount class and beginning to use that in places where it's most important, and gradually replacing CAmount, instead of replacing it everywhere? Or maybe there are other ways to split it up? Or maybe it is trivial to review and splitting it up would not be helpful? Curious what you think. This is just larger than I might have expected.

    I think the main reason why this looks large is commit c8fa9d5fbef02ccb2150448cb84ed6e9bba383ef and the two following ones. It should be trivial to split the them up and then reduce the size of this pull by two thirds. After this, it seems small enough and easy enough to do in one go.

  14. Sjors commented at 8:21 AM on June 17, 2026: member

    I think the main reason why this looks large is commit https://github.com/bitcoin/bitcoin/commit/c8fa9d5fbef02ccb2150448cb84ed6e9bba383ef and the two following ones.

    In addition to making b7af8d94d6708e50ebede9a513d006ac0031606c a followup or earlier PR (to introduce the _sats literal), it could be a scripted diff? The script is already in the commit description, but then CI can check it too.

  15. ryanofsky commented at 12:30 PM on June 17, 2026: contributor

    I think the main reason why this looks large is commit c8fa9d5 and the two following ones. It should be trivial to split the them up and then reduce the size of this pull by two thirds. After this, it seems small enough and easy enough to do in one go.

    Nice. I'd be ok with reviewing this all together, but IMO it would be good to introduce the _sats suffix as a standalone readability improvement so this PR is smaller and just focused on safety issues, which seem more subtle.

  16. hodlinator force-pushed on Jun 17, 2026
  17. hodlinator commented at 1:42 PM on June 17, 2026: contributor

    Thanks for the encouraging feedback & ideas to facilitate review! Migrating to a new class gradually is worth at least keeping in the back-pocket (https://github.com/bitcoin/bitcoin/pull/35511#issuecomment-4712614640). Agree with #35511 (comment) that peeling off the beginning half of this PR up until before CAmount starts becoming an actual class could make sense (everything before "ipc: Add coverage for passing CAmount"?).

    Regarding #35511 (comment), making a scripted diff commit out of the largest one ("refactor: Apply _sats literals (trivial)"): In that commit I'm preemptively manually inserting _sats in places which will otherwise no longer compile due to later commits. Outside of some kind of heavily C++ AST-aware Clang-integrated tooling, I don't see a way of doing that in a scripted fashion. Maybe there is some creative way of running the sed-script I have in reverse... I just cannot come up with it now, but I'm open to suggestions.

  18. maflcko commented at 1:48 PM on June 17, 2026: member

    Ah, it is an inverse scripted diff. In theory (after #35547) you could do an empty scripted diff in the next commit with:

    • Your sed command on ./src
    • git --no-pager diff --exit-code --quiet HEAD~
    • git checkout -- ./src

    Not sure if that is too spicy.

  19. DrahtBot added the label CI failed on Jun 17, 2026
  20. DrahtBot commented at 3:26 PM on June 17, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/27693235629/job/81909522457</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU reported include issues (“Failure generated from IWYU”), causing the test script to exit with status 1.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  21. optout21 commented at 9:16 AM on June 18, 2026: contributor

    A strong like for the concept of CAmount as a class rather than a simple typedef.

    These properties make a lot of sense:

    • constructible from a numerical value
    • convertible to to a numerical value, but only explicitly
    • automatic extra sanity checks (e.g. max)

    However, to be constructible only from _sats literals, and to be comparable only against other CAmount seems a bit of an overkill to me, and it hinders introduction (large change). An amount is a special kind of number, but it is a number, and I don't see the benefit of abstracting away that fact that it is a number.

    I would be happier to see a more gradual introduction:

    • constructible from a number (with internal max check)
    • using conversion to number for comparisons, computations (i.e., amount.Int() < 1000 instead of amount < 1000_sat
    • for an even more gradual introduction, as an intermediary step, the conversion to number can be automatic
    • forcing to exclusive comparison and arithmetics (as proposed currently) could be done as a later step.

    Also, I'm a bit uneasy with the allowed negative values. Maybe such a change would be a good time to separate absolute amounts and amount differences, with the former being strictly non-negative (e.g. CAmount andd CAmountDelta; an analogue would be time points and time differences). I suspect for most of the usages, non-negative absolute amount would fit.

  22. test: correct CAmount argument type for AddDuplicateCoins() 7897c90358
  23. fees: Use CAmount in FeeFrac where appropriate
    block_policy_estimator.cpp: Also static_cast to primitive type rather than CAmount.
    d45a4756da
  24. mempool: Make TxMempoolInfo::nFeeDelta a proper CAmount 7b73b7d6a1
  25. hodlinator commented at 11:45 AM on June 18, 2026: contributor

    Thanks for your feedback @optout21!

    constructible from a number (with internal max check) [...] I'm a bit uneasy with the allowed negative values. Maybe such a change would be a good time to separate absolute amounts and amount differences, with the former being strictly non-negative (e.g. CAmount andd CAmountDelta; an analogue would be time points and time differences). I suspect for most of the usages, non-negative absolute amount would fit.

    One of my first experiments in this area was having distinct Amount (signed) and UAmount (unsigned) types (older branch: https://github.com/hodlinator/bitcoin/tree/2026/03/safer_camount).

    After presenting this experiment at https://btctranscripts.com/bitcoin-core-dev-tech/2026-05/camount I got feedback about the pitfalls of unsigned integers (mixing signed with unsigned in the same expression makes the result unsigned, which might result in unintentional conversion from negative values to positive).

    This made me change my approach to only keep signed amounts, but to experiment with having an amount type which only allows [0,21M BTC] in addition to a type which allows any int64_t value. A first attempt in this direction is made here: https://github.com/hodlinator/bitcoin/tree/2026/06/safer_camount.take2.unchecked. But will probably go with a more careful approach as described in the final commit message.

    using conversion to number for comparisons, computations (i.e., amount.Int() < 1000 instead of amount < 1000_sat

    If we were going to introduce the requirement to use .Int() everywhere we might as well instead introduce the literal directly?

    • for an even more gradual introduction, as an intermediary step, the conversion to number can be automatic
    • forcing to exclusive comparison and arithmetics (as proposed currently) could be done as a later step.

    I briefly experimented with having an operator int64_t() method and allowing comparisons against integers. What I ran into a lot was that the compiler couldn't resolve the resulting ambiguities. Maybe there's a way to resolve those ambiguities, but my impression from earlier feedback is that the current approach without that intermediate step may be acceptable.

  26. hodlinator force-pushed on Jun 18, 2026
  27. hodlinator force-pushed on Jun 18, 2026
  28. mempool: TxGraph - Correct CAmount argument for SetTransactionFee() and SetFee() 6c7e77e17b
  29. consensus: Add _sats user defined literal
    The plan is to make CAmount more type safe in the future, so that it cannot be directly compared with random integer literals.
    d2c5ceb128
  30. refactor: Apply _sats literals (trivial)
    Next commit contains a scripted diff which proves we only inserted _sats-suffixes.
    c732c48f99
  31. scripted-diff: Prove the simplicity of previous commit
    Can be tested locally through running:
    cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=scripted_diff
    
    Verifiable through changing the sed-expression to no longer match _sats.
    
    -BEGIN VERIFY SCRIPT-
    sed -i -E 's/([0-9a-f])_sats\b/\1/g' $(git grep -l '_sats' ./src/) && \
    git --no-pager diff --exit-code --quiet HEAD~ && \
    git checkout -- ./src
    -END VERIFY SCRIPT-
    5efa0760fa
  32. refactor: Apply _sats literals (more manual)
    Also changes some implicitly bool conditions to != 0_sats.
    
    The reason "/*nValueIn=*/" was removed in src/wallet/test/fuzz/coinselection.cpp was that clang-tidy (mis)matched it with the v-argument for the _sats literal:
    /home/runner/work/_temp/src/wallet/test/fuzz/coinselection.cpp:377:45: error: argument name 'nValueIn' in comment does not match parameter name 'v' [bugprone-argument-comment,-warnings-as-errors]
      377 |     const auto dust{GetDustThreshold(CTxOut{/*nValueIn=*/0_sats, change_out_script}, coin_params.m_discard_feerate)};
          |                                             ^
    /home/runner/work/_temp/src/consensus/amount.h:136:53: note: 'v' declared here
      136 | constexpr Amount operator""_sats(unsigned long long v)
          |                                                     ^
    69fd3009a3
  33. test: Add _BTC literal to coinselector_tests.cpp fe04bd335b
  34. fuzz: Add ConsumeMoney() overloads with support for specifying minimum 65e657162a
  35. ipc: Add coverage for passing CAmount
    Co-authored-by: janb84 <githubjanb.drainer976@passmail.net>
    cf278519a4
  36. consensus: Make CAmount a class
    No longer allows implicit conversions from non-integer types such as floats or bools.
    
    Would prevent issues such as the one fixed by 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab.
    ed38d9ffa1
  37. refactor(mempool): Return nullopt instead of default-initialized TxMempoolInfo
    Preparation for CAmount not having default ctor.
    89df2ce723
  38. consensus: Require initialization of CAmount 8308afdd94
  39. consensus: Make CAmount(T) constructor explicit cc90782e0c
  40. hodlinator force-pushed on Jun 18, 2026
  41. DrahtBot removed the label CI failed on Jun 18, 2026

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-06-19 07:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me