refactor: migrate bool GetCoin to return optional<Coin> #30849

pull l0rinc wants to merge 3 commits into bitcoin:master from l0rinc:l0rinc/GetCoin-optional changing 16 files +99 −129
  1. l0rinc commented at 8:36 pm on September 8, 2024: contributor

    While reviewing the removal of the unreachable combinations from the Coin cache logic, we’ve noticed that the related tests often reflect impossible states.

    Browsing the Coin cache refactoring history revealed that migrating bool GetCoin to optional<Coin> GetCoin was already proposed a few times before.

    This refactor makes certain invalid states impossible, reducing the possibility of errors and making the code easier to understand. This will let us remove test code that exercises the impossible states as well. The PR is done in multiple small steps, first swapping the new optional return value, slowly strangling out the usages of the return parameter, followed by the removal of the parameter.

    Most of the invalid test states were still kept, except for https://github.com/bitcoin/bitcoin/pull/30673/files#r1748087322, where the new design prohibits invalid usage and https://github.com/bitcoin/bitcoin/pull/30673/files#r1749350258 was just marked with a TODO, will be removed in a follow-up PR.

  2. DrahtBot commented at 8:36 pm on September 8, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK andrewtoth, laanwj, theStack, achow101
    Concept ACK TheCharlatan, BrandonOdiwuor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30906 (refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests by l0rinc)
    • #30673 (coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries by andrewtoth)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)

    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. DrahtBot added the label Refactoring on Sep 8, 2024
  4. l0rinc renamed this:
    refactor: Remove unrealistic simulation state
    refactor: migrate `bool GetCoin` to return `optional<Coin>`
    on Sep 8, 2024
  5. in src/rest.cpp:869 in 8eae11b09a outdated 
    865@@ -866,11 +866,15 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
866     uint256 active_hash;
867     {
868         auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) {
869-            for (const COutPoint& vOutPoint : vOutPoints) {
870-                Coin coin;
871-                bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin);
872+            for (const COutPoint& vOutPoint: vOutPoints) {
    andrewtoth commented at 8:56 pm on September 8, 2024:
    nit: don’t remove space before :.
    l0rinc commented at 9:10 pm on September 8, 2024:
    Already done, forgot to push
  6. in src/test/coins_tests.cpp:53 in 8eae11b09a outdated 
    57-            return false;
58+        if (auto it{map_.find(outpoint)}; it != map_.end()) {
59+            if (!it->second.IsSpent() || m_rng.randbool()) {
60+                return it->second; // TODO spent coins shouldn't be returned
61+            } else {
62+                // Randomly return std::nullopt in case of an empty entry.
    andrewtoth commented at 9:02 pm on September 8, 2024:
    For clarity this comment should go above where we use m_rng. So it should go a few lines up and probably be changed to Randomly return spent coins.
    l0rinc commented at 9:09 pm on September 8, 2024:
    I’ll just remove it
  7. andrewtoth commented at 9:06 pm on September 8, 2024: contributor
    Concept ACK
  8. in src/test/fuzz/coinscache_sim.cpp:154 in 6243ceb65b outdated 
    150@@ -151,10 +151,6 @@ class CoinsViewBottom final : public CCoinsView
151     {
152         auto it = m_data.find(outpoint);
153         if (it == m_data.end()) {
154-            if ((outpoint.n % 5) == 3) {
    andrewtoth commented at 9:06 pm on September 8, 2024:

    In commit 6243ceb65b83ea8dc3a689fdcc868a3a94b86e80 refactor: Remove unrealistic simulation state

    The comment above should be removed in this commit as well, since it is no longer accurate.

    l0rinc commented at 9:10 pm on September 8, 2024:
    Done
  9. l0rinc force-pushed on Sep 8, 2024
  10. in src/test/fuzz/coinscache_sim.cpp:147 in ee75cc39bd outdated 
    140@@ -141,7 +141,7 @@ class CoinsViewBottom final : public CCoinsView
141     std::map<COutPoint, Coin> m_data;
142 
143 public:
144-    bool GetCoin(const COutPoint& outpoint, Coin& coin) const final
145+    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const final
146     {
147         auto it = m_data.find(outpoint);
148         if (it == m_data.end()) {
    TheCharlatan commented at 9:06 am on September 9, 2024:

    In commit ee75cc39bd8be72372c9b43f49a965225b737978:

    Not changing the return types here will lead to compile failure.

  11. in src/coins.cpp:12 in ee75cc39bd outdated 
     8@@ -9,20 +9,20 @@
 9 #include <random.h>
10 #include <util/trace.h>
11 
12-bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; }
13+std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint, Coin& coin) const { return std::nullopt; }
    TheCharlatan commented at 9:14 am on September 9, 2024:

    In commit ee75cc39bd8be72372c9b43f49a965225b737978

    There are places where you sometimes adjust the & spacing and where you leave it in its old style. I see that you adjust some of them in the following commits. Can you try to change them only on the first time you are changing a line?

    l0rinc commented at 11:03 am on September 9, 2024:
    Indeed, fixed
  12. TheCharlatan commented at 9:50 am on September 9, 2024: contributor

    Concept ACK

    Splitting up the removal of the Coin& parameter into another commit felt to me like it mostly introduced churn, without making review easier. I ended up diffing over both commits to look at the change.

  13. l0rinc force-pushed on Sep 9, 2024
  14. l0rinc force-pushed on Sep 9, 2024
  15. l0rinc force-pushed on Sep 9, 2024
  16. l0rinc force-pushed on Sep 9, 2024
  17. l0rinc commented at 11:04 am on September 9, 2024: contributor

    I ended up diffing over both commits to look at the change.

    Thanks, merged the last two commits

  18. in src/validation.cpp:188 in b53de0d5e3 outdated 
    186+        if (auto coin{coins.GetCoin(tx.vin[i].prevout)}) {
187+            prev_heights[i] = coin->nHeight == MEMPOOL_HEIGHT
188+                              ? tip.nHeight + 1 // Assume all mempool transaction confirm in the next block.
189+                              : coin->nHeight;
190+        } else {
191             LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
    l0rinc commented at 11:17 am on September 9, 2024:
    @maflcko should I use LogInfo here instead?
    maflcko commented at 11:33 am on September 9, 2024:

    @maflcko should I use LogInfo here instead?

    It can’t hurt, because it is a refactor. However, in some cases an alternative like LogWarning, LogError, Assume or Assert may be more useful. Also, it could make sense to explain why an error is unexpected and what should happen when they are encountered. Otherwise, users and developers wouldn’t know what to do if the error is encountered and whether it can be encountered at all.

    l0rinc commented at 11:49 am on September 9, 2024:
    Thanks, used LogError in a2a6f32 (#30849)
    maflcko commented at 12:34 pm on September 9, 2024:

    Maybe I am missing something, but commit a2a6f320878ddbedfca856d512d862fb46df4b23 says:

    0> "LogError(fmt, params...) should be used ... for severe problems that require the node ... to shut down entirely".
1
2Since this will throw a `TX_PREMATURE_SPEND` in `MemPoolAccept::PreChecks`, we'll use `LogError` instead.

    TX_PREMATURE_SPEND may happen in normal operation (for example, it happens in the tests) and does not require the node to shut down entirely. Also, the node does not shut down currently when TX_PREMATURE_SPEND happens.

    I haven’t looked into this change in detail, but at a minimum, the commit message doesn’t seem right.

    Edit: Also, changing the log severity is not a “refactor”.

    l0rinc commented at 1:04 pm on September 9, 2024:

    Also, changing the log severity is not a “refactor”.

    K, dropping this commit in that case

  19. l0rinc force-pushed on Sep 9, 2024
  20. l0rinc force-pushed on Sep 9, 2024
  21. l0rinc force-pushed on Sep 9, 2024
  22. DrahtBot added the label CI failed on Sep 9, 2024
  23. l0rinc force-pushed on Sep 9, 2024
  24. BrandonOdiwuor commented at 10:18 am on September 11, 2024: contributor

    Concept ACK

    Using an optional return value is a cleaner approach compared to a boolean return with an output parameter.

  25. in src/test/fuzz/coinscache_sim.cpp:142 in d665bcc2c9 outdated 
    134@@ -135,13 +135,7 @@ struct CacheLevel
135     }
136 };
137 
138-/** Class for the base of the hierarchy (roughly simulating a memory-backed CCoinsViewDB).
139- *
140- * The initial state consists of the empty UTXO set, though coins whose output index
141- * is 3 (mod 5) always have GetCoin() succeed (but returning an IsSpent() coin unless a UTXO
142- * exists). Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent.
    andrewtoth commented at 5:02 pm on September 11, 2024:
    This part Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent is due to us not erasing these coins with this condition on line 169 below. Since we are not yet not returning spent coins (// TODO GetCoin shouldn't return spent coins), we shouldn’t remove this line of the comment.
    l0rinc commented at 5:19 pm on September 11, 2024:
    Done, thanks
  26. in src/coins.cpp:29 in db33db3cf5 outdated 
    31+CCoinsViewBacked::CCoinsViewBacked(CCoinsView* viewIn) : base(viewIn) {}
32+std::optional<Coin> CCoinsViewBacked::GetCoin(const COutPoint& outpoint, Coin& coin) const { return base->GetCoin(outpoint, coin); }
33+bool CCoinsViewBacked::HaveCoin(const COutPoint& outpoint) const { return base->HaveCoin(outpoint); }
34 uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); }
35 std::vector<uint256> CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); }
36-void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; }
    andrewtoth commented at 5:07 pm on September 11, 2024:
    I don’t think we should be changing all these lines in this commit just for updating the style. It muddies the git blame.
    l0rinc commented at 5:24 pm on September 11, 2024:
    Fair, reverted the unrelated lines
  27. andrewtoth changes_requested
  28. l0rinc force-pushed on Sep 11, 2024
  29. in src/coins.cpp:53 in 376ba5fcfd outdated 
    49+            if (coin->IsSpent()) { // TODO GetCoin cannot return spent coins
50+                // The parent only has an empty entry for this outpoint; we can consider our version as fresh.
51+                ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel);
52+            }
53+            cachedCoinsUsage += coin->DynamicMemoryUsage();
54+            ret->second.coin = std::move(*coin);
    andrewtoth commented at 10:22 pm on September 11, 2024:

    This line should be moved up to just under the if statement to keep behavior the same. The coin is already moved into ret->second.coin before we potentially call AddFlags.

    Although this will hopefully go away in #30673.

    l0rinc commented at 8:08 am on September 12, 2024:
    I don’t think there’s any difference (except for having to use ret->second.coin.DynamicMemoryUsage and ret->second.coin.IsSpent() instead), but I don’t mind moving it up.
  30. in src/coins.cpp:18 in c6d003cd56 outdated 
    15 std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
16 bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; }
17 std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }
18 
19-bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
20+bool CCoinsView::HaveCoin(const COutPoint& outpoint) const
    andrewtoth commented at 10:39 pm on September 11, 2024:
    I don’t think we should be changing HaveCoin lines in a commit about GetCoin.
    l0rinc commented at 8:06 am on September 12, 2024:
    I did modify HaveCoin in this commit - but since you think it’s noise, I’ve reverted
  31. in src/coins.cpp:26 in c6d003cd56 outdated 
    26 
27 CCoinsViewBacked::CCoinsViewBacked(CCoinsView *viewIn) : base(viewIn) { }
28-bool CCoinsViewBacked::GetCoin(const COutPoint &outpoint, Coin &coin) const { return base->GetCoin(outpoint, coin); }
29-bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base->HaveCoin(outpoint); }
30+std::optional<Coin> CCoinsViewBacked::GetCoin(const COutPoint& outpoint, Coin& coin) const { return base->GetCoin(outpoint, coin); }
31+bool CCoinsViewBacked::HaveCoin(const COutPoint& outpoint) const { return base->HaveCoin(outpoint); }
    andrewtoth commented at 10:39 pm on September 11, 2024:
    Same re: HaveCoin
  32. in src/coins.cpp:387 in c6d003cd56 outdated 
    389+    return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks);
390 }
391 
392-bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint &outpoint) const {
393-    return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
394+bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const
    andrewtoth commented at 10:40 pm on September 11, 2024:
    Same re: HaveCoin
  33. in src/txdb.h:63 in c6d003cd56 outdated 
    58@@ -59,8 +59,8 @@ class CCoinsViewDB final : public CCoinsView
59 public:
60     explicit CCoinsViewDB(DBParams db_params, CoinsViewOptions options);
61 
62-    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
63-    bool HaveCoin(const COutPoint &outpoint) const override;
64+    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
65+    bool HaveCoin(const COutPoint& outpoint) const override;
    andrewtoth commented at 10:40 pm on September 11, 2024:
    Same re: HaveCoin
  34. andrewtoth changes_requested
  35. DrahtBot removed the label CI failed on Sep 11, 2024
  36. l0rinc force-pushed on Sep 12, 2024
  37. l0rinc force-pushed on Sep 12, 2024
  38. DrahtBot added the label CI failed on Sep 12, 2024
  39. in src/coins.cpp:387 in d2146c317b outdated 
    386+    return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint); }, m_err_callbacks);
387 }
388 
389-bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint &outpoint) const {
390-    return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
391+bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint &outpoint) const
    andrewtoth commented at 2:43 pm on September 12, 2024:
    nit: this one line doesn’t need to be changed and have the brace moved.
    l0rinc commented at 2:54 pm on September 12, 2024:
    I’ll revert if more changes are needed
    andrewtoth commented at 5:47 pm on September 18, 2024:
    Hmm looks like you moved the & now too, along with the brace on the newline. I don’t see any change in the header in that diff.
    l0rinc commented at 6:02 pm on September 18, 2024:

    Yes, formatted the cpp, since it was modified, reverted the header, since it wasn’t.

    Resolving this comment, as the developer notes state:

    However, we’re now trying to converge to a single style, which is specified below. When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.

    But let me know if it was correct before, and my modification is incorrect, maybe I misunderstood what you meant.

    andrewtoth commented at 1:37 am on September 19, 2024:

    I meant that the HaveCoin signature is not relevant to this PR. So, just modifying it by moving the brace to the next line and moving the & to be next to the type is making a style change to a line that doesn’t need to be touched in this PR.

    If a line must be changed for the PR anyways, then yes I agree the style of the entire line should be updated.

  40. andrewtoth approved
  41. andrewtoth commented at 2:46 pm on September 12, 2024: contributor

    Code review ACK d2146c317b63f635e3683cb6e7d0913be6c96f3f

    This change simplifies the GetCoin interface, so callers don’t need to consider both the return value and an out parameter. This lets us remove test code that exercises different combinations of the return value and out parameter, which will let us make test coverage simpler and more sane.

  42. DrahtBot requested review from BrandonOdiwuor on Sep 12, 2024
  43. DrahtBot requested review from TheCharlatan on Sep 12, 2024
  44. DrahtBot removed the label CI failed on Sep 13, 2024
  45. theStack commented at 12:08 pm on September 17, 2024: contributor
    Concept ACK
  46. in src/node/coin.cpp:21 in d2146c317b outdated 
    21-            // Either the coin is not in the CCoinsViewCache or is spent. Clear it.
22-            coin.second.Clear();
23-        }
24+    for (auto& [outpoint, coin] : coins) {
25+        if (auto c{mempool_view.GetCoin(outpoint)}) coin = std::move(*c);
26+        else coin.Clear(); // Either the coin is not in the CCoinsViewCache or is spent
    hodlinator commented at 7:20 am on September 18, 2024:

    developer-notes.md:

    If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line.

    l0rinc commented at 1:10 pm on September 18, 2024:
    Sir, yes sir!
  47. in src/rest.cpp:877 in d2146c317b outdated 
    874+                    if (auto coin{view.GetCoin(vOutPoint)}) {
875+                        outs.emplace_back(std::move(*coin));
876+                        hit = true;
877+                    }
878+                }
879                 hits.push_back(hit);
    hodlinator commented at 7:26 am on September 18, 2024:

    Avoid inflating the line count?

    0                std::optional<Coin> coin;
1                if (!mempool || !mempool->isSpent(vOutPoint)) coin = view.GetCoin(vOutPoint);
2                hits.push_back(coin.has_value());
3                if (coin) outs.emplace_back(std::move(*coin));
    l0rinc commented at 12:58 pm on September 18, 2024:
    Nice, added a ternary to simplify further, thanks!
  48. in src/test/fuzz/coinscache_sim.cpp:153 in d2146c317b outdated 
    163-            coin = it->second;
164-            return true;
165-        }
166+        // TODO GetCoin shouldn't return spent coins
167+        if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second;
168+        else return std::nullopt;
    hodlinator commented at 7:33 am on September 18, 2024:
    ++braces, or remove the else.
    l0rinc commented at 1:11 pm on September 18, 2024:
    removed the else
  49. in src/txdb.cpp:71 in d2146c317b outdated 
    64@@ -65,8 +65,10 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size)
65     }
66 }
67 
68-bool CCoinsViewDB::GetCoin(const COutPoint &outpoint, Coin &coin) const {
69-    return m_db->Read(CoinEntry(&outpoint), coin);
70+std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint) const
71+{
72+    if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) return coin;
73+    else return std::nullopt;
    hodlinator commented at 7:36 am on September 18, 2024:
    *taps sign*
  50. in src/coins.cpp:65 in d2146c317b outdated 
    73-    }
74-    return false;
75+std::optional<Coin> CCoinsViewCache::GetCoin(const COutPoint& outpoint) const
76+{
77+    if (auto it{FetchCoin(outpoint)}; it != cacheCoins.end() && !it->second.coin.IsSpent()) return it->second.coin;
78+    else return std::nullopt;
    hodlinator commented at 7:40 am on September 18, 2024:
    *taps sign*
  51. hodlinator commented at 8:00 am on September 18, 2024: contributor

    Drive-by minimum-context, admittedly low-PoW review, prompted by author.

    Dislike adding of new TODO’s but if full-context reviewers accept, I guess it’s okay.

  52. l0rinc force-pushed on Sep 18, 2024
  53. l0rinc force-pushed on Sep 18, 2024
  54. l0rinc force-pushed on Sep 18, 2024
  55. l0rinc force-pushed on Sep 18, 2024
  56. refactor: Remove unrealistic simulation state 
    In non-test code the input coin is never mutated - it's either replaced or ignored.
    e31bfb26c2
  57. refactor: Return optional of Coin in GetCoin 
    Leaving the parameter as well for now.

Co-authored-by: TheCharlatan <seb.kung@gmail.com>
    46dfbf169b
  58. refactor: Rely on returned value of GetCoin instead of parameter 
    Also removed the unused coin parameter of GetCoin.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
    4feaa28728
  59. l0rinc force-pushed on Sep 18, 2024
  60. andrewtoth commented at 1:34 am on September 19, 2024: contributor
    re-ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
  61. DrahtBot requested review from theStack on Sep 19, 2024
  62. laanwj approved
  63. laanwj commented at 12:06 pm on October 23, 2024: member
    Code review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0 Not the easiest refactor to review, carefully checked that there are no behavior differences, but i think the change absolutely worth it for the more readable API, and the removal of ambiguity.
  64. in src/rest.cpp:873 in 4feaa28728 
    869@@ -870,10 +870,9 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
870     {
871         auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) {
872             for (const COutPoint& vOutPoint : vOutPoints) {
873-                Coin coin;
874-                bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin);
875-                hits.push_back(hit);
876-                if (hit) outs.emplace_back(std::move(coin));
877+                auto coin = !mempool || !mempool->isSpent(vOutPoint) ? view.GetCoin(vOutPoint) : std::nullopt;
    theStack commented at 4:05 pm on October 24, 2024:

    nit, if you retouch:

    0                auto coin = (!mempool || !mempool->isSpent(vOutPoint)) ? view.GetCoin(vOutPoint) : std::nullopt;

    (slightly easier to read imho; it’s no big deal though here, maybe i’m just post-traumatized by the serialized hash calculation bug where missing parantheses in a ternary expression were an issue :P)

  65. in src/test/util/setup_common.cpp:436 in 4feaa28728 
    431@@ -432,9 +432,8 @@ std::pair<CMutableTransaction, CAmount> TestChain100Setup::CreateValidTransactio
432     std::map<COutPoint, Coin> input_coins;
433     CAmount inputs_amount{0};
434     for (const auto& outpoint_to_spend : inputs) {
435-        // - Use GetCoin to properly populate utxo_to_spend,
436-        Coin utxo_to_spend;
437-        assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend));
438+        // Use GetCoin to properly populate utxo_to_spend
439+        auto utxo_to_spend{coins_cache.GetCoin(outpoint_to_spend).value()};
    theStack commented at 4:11 pm on October 24, 2024:
    nit: the assert is lost here, but since std::optional’s value method throws an exception if no value is available, it seems to be fine to not have it (I assume that was intentional for that reason)
    l0rinc commented at 4:30 pm on October 24, 2024:
    I remember this being asked before, but I can’t find it - yes, .value() already validates the optional.
  66. theStack approved
  67. theStack commented at 4:11 pm on October 24, 2024: contributor
    Code-review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
  68. achow101 commented at 5:04 pm on October 24, 2024: member
    ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
  69. achow101 merged this on Oct 24, 2024
  70. achow101 closed this on Oct 24, 2024
  71. l0rinc deleted the branch on Oct 24, 2024


l0rinc DrahtBot andrewtoth TheCharlatan maflcko BrandonOdiwuor theStack hodlinator laanwj achow101


TheCharlatan BrandonOdiwuor

Labels
Refactoring

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: 2024-11-23 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me