refactor: Use move semantics instead of custom swap functions #26749

hebasto commented at 11:38 PM on December 24, 2022: member

This PR makes code more succinct and readable by using move semantics.

DrahtBot commented at 11:39 PM on December 24, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	martinus, achow101, TheCharlatan, MarcoFalke
Concept ACK	sipa, aureleoules
Stale ACK	ryanofsky

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Refactoring on Dec 24, 2022

fanquake requested review from ryanofsky on Dec 28, 2022

sipa commented at 1:16 PM on December 31, 2022: member

Concept ACK

aureleoules approved

aureleoules commented at 11:12 AM on January 2, 2023: contributor

Concept ACK

in src/validation.cpp:1776 in 78030e0c08 outdated

1731 | @@ -1732,8 +1732,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
1732 |          // Verify signature
1733 |          CScriptCheck check(txdata.m_spent_outputs[i], tx, i, flags, cacheSigStore, &txdata);
1734 |          if (pvChecks) {
1735 | -            pvChecks->push_back(CScriptCheck());
1736 | -            check.swap(pvChecks->back());
1737 | +            pvChecks->push_back(std::move(check));

maflcko commented at 5:03 PM on January 3, 2023:

Review note in 78030e0c08ce8ab30bde05ce97def571f05f1399: Calling the swap member function ensured at compile time that the object is not const. Now it is done at compile time only with clang-tidy performance-move-const-arg?

hebasto commented at 7:34 PM on January 3, 2023:

Updated. Move semantics forced by making CScriptCheck a non-copyable class.

ryanofsky commented at 3:49 PM on March 2, 2023:

In commit "consensus, refactor: Avoid CScriptCheck::swap in CheckInputScripts" (97292dafb528d2a097f29e4c79a08cced49e5451)

Should this use emplace_back instead of push_back?

maflcko commented at 9:18 AM on March 3, 2023:

Should this use emplace_back instead of push_back?

I didn't mention this because I assumed the two functions would do the same if they received a moved object that is already fully constructed?

ryanofsky commented at 1:06 PM on March 3, 2023:

Should this use emplace_back instead of push_back?

I didn't mention this because I assumed the two functions would do the same if they received a moved object that is already fully constructed?

Oh, I think you are right they are equivalent in this case. I still think it would be a little better to use emplace_back for readability though, because with emplace_back you can be sure object is being constructed directly, while with push_back you have to check the types to know if a temporary is being created because of an implicit conversion.

hebasto commented at 4:16 PM on March 16, 2023:

Thanks! Updated.

in src/validation.h:306 in 78030e0c08 outdated

 302 | @@ -303,6 +303,8 @@ class CScriptCheck
 303 |      ScriptError GetScriptError() const { return error; }
 304 |  };
 305 |  
 306 | +static_assert(std::is_move_constructible_v<CScriptCheck>);

maflcko commented at 5:04 PM on January 3, 2023:

78030e0c08ce8ab30bde05ce97def571f05f1399: Not sure what the point of this is, given that it will be even true if there is a copy constructor.

hebasto commented at 7:35 PM on January 3, 2023:

Thanks! Updated.

in src/checkqueue.h:32 in 68d4728ff4 outdated

  28 | @@ -29,6 +29,8 @@ class CCheckQueueControl;
  29 |  template <typename T>
  30 |  class CCheckQueue
  31 |  {
  32 | +    static_assert(std::is_move_constructible_v<T>);

maflcko commented at 5:05 PM on January 3, 2023:

68d4728ff4d0fd14aa944b8d67a47c2fbecd980b same

hebasto commented at 7:35 PM on January 3, 2023:

Thanks! Updated.

in src/test/checkqueue_tests.cpp:127 in 68d4728ff4 outdated

 138 | @@ -139,6 +139,11 @@ struct FrozenCleanupCheck {
 139 |              cv.wait(l, []{ return nFrozen.load(std::memory_order_relaxed) == 0;});
 140 |          }
 141 |      }
 142 | +    FrozenCleanupCheck(FrozenCleanupCheck&& other) noexcept

maflcko commented at 5:21 PM on January 3, 2023:

in 68d4728ff4d0fd14aa944b8d67a47c2fbecd980b: Replace // swaps in default initialized Checks with // moves in default initialized Checks?

hebasto commented at 7:36 PM on January 3, 2023:

Thanks! Updated.

in src/checkqueue.h:114 in 9d454e8447 outdated

 112 | @@ -113,11 +113,8 @@ class CCheckQueue
 113 |                  // * Try to account for idle jobs which will instantly start helping.
 114 |                  // * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
 115 |                  nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
 116 | -                vChecks.resize(nNow);

maflcko commented at 5:28 PM on January 3, 2023:

9d454e8447456c91ec45512d0caaa71bd0bb8c0f: Changing resize into push_back (with implicit resize) seems like a unrelated behaviour change that violates the comment you removed?

hebasto commented at 7:05 PM on January 3, 2023:

@MarcoFalke Does your comment consider that the required memory has been pre-allocated already: https://github.com/bitcoin/bitcoin/blob/cb552c5f21192d2f8f69c07130928c21301a5e7f/src/checkqueue.h#L73?

maflcko commented at 5:29 PM on January 3, 2023: member

ACK 3bc1cd3b4ae41182de0c05339adb2a2ad6774aeb 🍈

<details><summary>Show signature</summary>

Signature:

</details>

hebasto force-pushed on Jan 3, 2023

hebasto commented at 7:33 PM on January 3, 2023: member

Updated 3bc1cd3b4ae41182de0c05339adb2a2ad6774aeb -> 94a0f7f0d1168c77b6dac3ca3723ddcd7837fa31 (pr26749.01 -> pr26749.02, diff):

addressed @MarcoFalke's comments

in src/bench/checkqueue.cpp:62 in 39a53bb7db outdated

  58 | @@ -60,7 +59,7 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench)
  59 |      bench.minEpochIterations(10).batch(BATCH_SIZE * BATCHES).unit("job").run([&] {
  60 |          // Make insecure_rand here so that each iteration is identical.
  61 |          CCheckQueueControl<PrevectorJob> control(&queue);
  62 | -        for (auto vChecks : vBatches) {
  63 | +        for (auto&& vChecks : vBatches) {

maflcko commented at 3:03 PM on January 6, 2023:

3f3479c5be1cd868d51c166a3808565afc56fb9f: This looks wrong? Previously it would create a copy. Now it steals the memory and then continues to iterate on the "empty" memory?

hebasto commented at 3:46 PM on January 15, 2023:

Updated.

maflcko commented at 9:51 AM on January 16, 2023:

You didn't change anything?? Pretty sure & or && are compiled down to the same (wrong) binary.

Otherwise, it would be good if you explained how your change changed anything or provide a benchmark output, etc.

hebasto commented at 11:33 AM on January 16, 2023:

... then continues to iterate on the "empty" memory?

Why? I don't think that is the case.

$ ./src/bench/bench_bitcoin --filter=CCheckQueueSpeedPrevectorJob
...
|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              161.43 |        6,194,675.97 |    2.4% |      0.06 | `CCheckQueueSpeedPrevectorJob`

It is easy to modify PrevectorJob::operator() by adding some observable effects, for example a delay or printing out, to verify correctness.

maflcko commented at 11:54 AM on January 16, 2023:

vBatches is not const, so auto& or auto&& create a mutable reference. Passing this to Add, which takes a mutable reference and calls std::move on it to cast it to enable move semantics means it will move the memory. auto created a copy.

You posted a single bench run, without a run on master there is not much info to be gathered.

hebasto commented at 1:20 PM on January 16, 2023:

vBatches is not const, so auto& or auto&& create a mutable reference. Passing this to Add, which takes a mutable reference and calls std::move on it to cast it to enable move semantics means it will move the memory. auto created a copy.

Correct. But it does not imply that the loop "... continues to iterate on the "empty" memory ", no?

You posted a single bench run, without a run on master there is not much info to be gathered.

Master branch (f3bc1a72825fe2b51f4bc20e004cef464f05b965):

|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              236.18 |        4,233,975.84 |    1.3% |      0.09 | `CCheckQueueSpeedPrevectorJob`

This PR (53b03946e4dad43f10ba2bd869f754555403c097):

|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              171.49 |        5,831,189.43 |    1.7% |      0.07 | `CCheckQueueSpeedPrevectorJob`

maflcko commented at 1:34 PM on January 16, 2023:

It implies that the state of the vector is unspecified, but valid. (Sometimes it is valid and empty, but not required to be empty). See https://en.cppreference.com/w/cpp/utility/move and the bench output you posted.

hebasto commented at 1:38 PM on January 16, 2023:

Right, the state of the local vBatches is unspecified after finishing the for-loop. But instances moved into control are perfectly valid.

maflcko commented at 1:44 PM on January 16, 2023:

The for loop can be run more than once, so they are unspecified before starting the loop

hebasto commented at 1:49 PM on January 16, 2023:

The for loop can be run more than once, so they are unspecified before starting the loop

Oh, right... :man_facepalming:

hebasto commented at 2:01 PM on January 16, 2023:

Thanks! Fixed.

This PR (cbd2e1c3f9fec7e14c04f5f6ddb4ee41e32e9c9c):

|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              228.13 |        4,383,416.74 |    1.8% |      0.08 | `CCheckQueueSpeedPrevectorJob`

maflcko approved

maflcko commented at 3:19 PM on January 6, 2023: member

review ACK 94a0f7f0d1 🗓

<details><summary>Show signature</summary>

Signature:

</details>

hebasto force-pushed on Jan 15, 2023

hebasto commented at 3:45 PM on January 15, 2023: member

Updated 94a0f7f0d1168c77b6dac3ca3723ddcd7837fa31 -> 1e756af73431724990e893864eb3124ee07cee61 (pr26749.02 -> pr26749.03, diff):

addressed @MarcoFalke's comment

hebasto renamed this:
~~refactor: Use move semanitcs in favour of custom swap functions~~
refactor: Use move semanitcs instead of custom swap functions
on Jan 15, 2023

in src/checkqueue.h:170 in 356b2a5b94 outdated

 172 | @@ -173,10 +173,7 @@ class CCheckQueue
 173 |

maflcko commented at 9:47 AM on January 16, 2023:

in 356b2a5b946304165a8f762dc6a2d62f3e12139d: I don't really like using move semantics on a mutable reference. This makes code fragile, because the caller may not realize the memory is stolen. It would be better to adjust the signature of this function from & to &&.

hebasto commented at 11:20 AM on January 16, 2023:

Agree.

hebasto commented at 1:01 PM on January 16, 2023:

It would be better to adjust the signature of this function from & to &&.

Thanks! Added a dedicated commit.

maflcko approved

maflcko commented at 9:52 AM on January 16, 2023: member

re-ACK 1e756af73431724990e893864eb3124ee07cee61 👃

<details><summary>Show signature</summary>

Signature:

</details>

in src/bench/checkqueue.cpp:58 in cbf4e59257 outdated

  59 | @@ -60,7 +60,7 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench)
  60 |          // Make insecure_rand here so that each iteration is identical.
  61 |          CCheckQueueControl<PrevectorJob> control(&queue);
  62 |          for (auto& vChecks : vBatches) {
  63 | -            control.Add(vChecks);
  64 | +            control.Add(std::move(vChecks));

maflcko commented at 1:00 PM on January 16, 2023:

nit? Would be nice to re-order this commit right after the Add commit

hebasto commented at 1:11 PM on January 16, 2023:

Reordered.

hebasto force-pushed on Jan 16, 2023

maflcko commented at 1:45 PM on January 16, 2023: member

est/checkqueue_tests.cpp:165:13: error: 'vChecks' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
            vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
            ^
test/checkqueue_tests.cpp:167:21: note: move occurred here
            control.Add(std::move(vChecks));
                    ^

hebasto force-pushed on Jan 16, 2023

hebasto marked this as a draft on Jan 16, 2023

hebasto force-pushed on Jan 16, 2023

hebasto marked this as ready for review on Jan 16, 2023

hebasto closed this on Jan 16, 2023

hebasto reopened this on Jan 16, 2023

hebasto commented at 3:24 PM on January 16, 2023: member

est/checkqueue_tests.cpp:165:13: error: 'vChecks' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
            vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
            ^
test/checkqueue_tests.cpp:167:21: note: move occurred here
            control.Add(std::move(vChecks));
                    ^

Fixed. No significant changes in the output of time ./src/test/test_bitcoin -t checkqueue_tests/test_CheckQueue_Correct_* command.

fanquake renamed this:
~~refactor: Use move semanitcs instead of custom swap functions~~
refactor: Use move semantics instead of custom swap functions
on Jan 16, 2023

hebasto commented at 4:41 PM on January 16, 2023: member

I'm not sure what is wrong with CI-GH integration...

CI: https://cirrus-ci.com/build/5532411732164608

maflcko added the label Needs rebase on Jan 18, 2023

hebasto force-pushed on Jan 18, 2023

hebasto commented at 3:41 PM on January 18, 2023: member

Needs rebase

Rebased 39b38b8054149e753c4f113410567d948e37e054 -> 142718890e718397e0026c315c8940102b9657ce (pr26749.07 -> pr26749.08).

DrahtBot removed the label Needs rebase on Jan 18, 2023

in src/test/checkqueue_tests.cpp:175 in bee19efe7d outdated

 174 | @@ -170,14 +175,12 @@ static void Correct_Queue_range(std::vector<size_t> range)
 175 |  {
 176 |      auto small_queue = std::make_unique<Correct_Queue>(QUEUE_BATCH_SIZE);
 177 |      small_queue->StartWorkerThreads(SCRIPT_CHECK_THREADS);
 178 | -    // Make vChecks here to save on malloc (this test can be slow...)
 179 | -    std::vector<FakeCheckCheckCompletion> vChecks;

ryanofsky commented at 4:56 PM on March 2, 2023:

In commit "refactor: Use move semantics in CCheckQueue::Add" (bee19efe7d8f1d6c6e2be0eefa9745de2d42066d)

Previous code seems to be trying to boost performance by reusing the vector. Why is it changed here to not reuse the vector? Change seems orthogonal to the rest of the PR

hebasto commented at 4:17 PM on March 16, 2023:

~Thanks! Updated.~

hebasto commented at 4:26 PM on March 16, 2023:

Well, the reason of that change is to avoid using of vChecks after it was moved:

test/checkqueue_tests.cpp:166:13: error: 'vChecks' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
            vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
            ^
test/checkqueue_tests.cpp:168:21: note: move occurred here
            control.Add(std::move(vChecks));
                    ^

maflcko commented at 5:06 PM on March 16, 2023:

bug fixes should be separate from refactoring commits

hebasto commented at 5:46 PM on March 16, 2023:

bug fixes should be separate from refactoring commits

Commits have been reorganized.

maflcko commented at 10:00 AM on March 20, 2023:

Commits have been reorganized.

No they haven't?

maflcko commented at 10:01 AM on March 20, 2023:

The commit that fixes the use-after-free should come with a commit message to say that it fixed the bug

hebasto commented at 1:36 PM on March 20, 2023:

The commit that fixes the use-after-free should come with a commit message to say that it fixed the bug

Thanks! Updated.

maflcko commented at 10:16 AM on March 21, 2023:

in commit 92dbf055528f16fcf51ea4bb310b9f319796d57f

I took another look at this and it seems this isn't a bugfix, but a workaround to silence a clang-tidy warning.

My understanding is that FakeCheckCheckCompletion can't be moved/swapped more efficiently than (default)constructing it, since it is just an empty struct. However, the underlying vector that holds the objects still needs to malloc (according to the comment you deleted).

I think an alternative fix would be to simply call clear() before resize(), and maybe even reserve()/construct with the max capacity on construction. This should reduce malloc to a single call at vChecks construction. Also, this should make clang-tidy happy, because of clear(). Finally, clear() should not change the capacity.

maflcko commented at 10:31 AM on March 21, 2023:

(No idea if this makes a difference, the compiler may optimize everything into the same binary anyway)

hebasto commented at 11:36 AM on March 21, 2023:

I think an alternative fix would be to simply call clear() before resize(), and maybe even reserve()/construct with the max capacity on construction. This should reduce malloc to a single call at vChecks construction. Also, this should make clang-tidy happy, because of clear(). Finally, clear() should not change the capacity.

Thanks! Updated.

ryanofsky approved

ryanofsky commented at 10:27 PM on March 2, 2023: contributor

Conditional code review ACK 142718890e718397e0026c315c8940102b9657ce, assuming this shouldn't use emplace_back #26749 (review) or avoid temporary vector allocations #26749 (review).

I mostly reviewed the overall PR diff and didn't review individual commits in detail since it was easier to think about class definitions with only move operations or only swap operations, not combinations of both.

DrahtBot requested review from maflcko on Mar 2, 2023

sedited commented at 11:27 AM on March 6, 2023: contributor

Light code review ACK 142718890e718397e0026c315c8940102b9657ce

I mostly reviewed the overall PR diff and didn't review individual commits in detail since it was easier to think about class definitions with only move operations or only swap operations, not combinations of both.

I can echo this, reviewing this piecemeal was a bit more difficult than just looking at it all at once.

fanquake commented at 9:07 AM on March 7, 2023: member

ACKs here, but these two comments should be addressed:

assuming this shouldn't use emplace_back #26749 (review) or avoid temporary vector allocations #26749 (review).

maflcko commented at 7:40 PM on March 7, 2023: member

Changes:

Adding a wrong hunk? #26749 (review)
Fixing my feedback from #26749 (review)
Adding commit 7673fd0b41

re-ACK 142718890e718397e0026c315c8940102b9657ce 📬

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 142718890e718397e0026c315c8940102b9657ce 📬
etOecIVx96PfsoYK8ZLgA7BJj/R+E8aK8aMZuZJrFV9XCD7hp70+tEK2Y/EjktxsRIaXTEj8sT7p1NWB59hRDA==

</details>

DrahtBot removed review request from maflcko on Mar 8, 2023

DrahtBot requested review from maflcko on Mar 8, 2023

DrahtBot removed review request from maflcko on Mar 8, 2023

hebasto force-pushed on Mar 16, 2023

hebasto commented at 4:16 PM on March 16, 2023: member

Updated 142718890e718397e0026c315c8940102b9657ce -> 4b31ef43fa198a0618fd61da121bd6f59e8be7f6 (pr26749.08 -> pr26749.09, diff).

Addressed @ryanofsky's comments:

hebasto marked this as a draft on Mar 16, 2023

hebasto force-pushed on Mar 16, 2023

hebasto marked this as ready for review on Mar 16, 2023

hebasto force-pushed on Mar 16, 2023

in src/checkqueue.h:176 in 852ed596a6 outdated

 176 |              LOCK(m_mutex);
 177 | -            for (T& check : vChecks) {
 178 | -                queue.emplace_back();
 179 | -                check.swap(queue.back());
 180 | -            }
 181 | +            std::move(vChecks.begin(), vChecks.end(), std::back_inserter(queue));

martinus commented at 6:36 PM on March 19, 2023:

I'd do it this way:

queue.insert(queue.end(), std::make_move_iterator(vChecks.begin()), std::make_move_iterator(vChecks.end()));

This has the advantage that queue's insert knows the number of elements that are going to be inserted and will reserve enough space, so it's faster

hebasto commented at 1:36 PM on March 20, 2023:

Thanks! Updated.

in src/checkqueue.h:114 in 899b1e630a outdated

 110 | @@ -111,11 +111,8 @@ class CCheckQueue
 111 |                  // * Try to account for idle jobs which will instantly start helping.
 112 |                  // * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
 113 |                  nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
 114 | -                vChecks.resize(nNow);
 115 |                  for (unsigned int i = 0; i < nNow; i++) {

martinus commented at 6:41 PM on March 19, 2023:

Instead of the loop I'd use this bulk method:

auto start_it = queue.end() - nNow;
vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end()));
queue.erase(start_it, queue.end());

Again that should be faster than lots of calls to push_back which need to check the capacity, also I find it more readable. Note that the difference is that the queue items will be added in a non-reversed order, but I'd say that should be irrelevant.

I gave that a try, and to my surprise this interestingly doesn't compile, for these reasons:

The simple std::move here is more or less a best-effort, when the object can't be moved, it will be copied. That is not the case when using std::make_move_iterator, it has a static_assert that the object must be move-assignable. So actually for DumbCheck, which is actually copied because it has a const bool member. Another issue is that FrozenCleanupCheck doesn't implement move-assignment.

So full change set should be this:

diff --git a/src/checkqueue.h b/src/checkqueue.h
index cb751d07f0..a309372ac8 100644
--- a/src/checkqueue.h
+++ b/src/checkqueue.h
@@ -111,10 +111,9 @@ private:
                 // * Try to account for idle jobs which will instantly start helping.
                 // * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
                 nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
-                for (unsigned int i = 0; i < nNow; i++) {
-                    vChecks.push_back(std::move(queue.back()));
-                    queue.pop_back();
-                }
+                auto start_it = queue.end() - nNow;
+                vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end()));
+                queue.erase(start_it, queue.end());
                 // Check whether we need to do work at all
                 fOk = fAllOk;
             }
@@ -170,7 +169,7 @@ public:
 
         {
             LOCK(m_mutex);
-            std::move(vChecks.begin(), vChecks.end(), std::back_inserter(queue));
+            queue.insert(queue.end(), std::make_move_iterator(vChecks.begin()), std::make_move_iterator(vChecks.end()));
             nTodo += vChecks.size();
         }
 
diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 9954780a7a..5c4ec55da3 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -128,6 +128,12 @@ struct FrozenCleanupCheck {
         should_freeze = other.should_freeze;
         other.should_freeze = false;
     }
+    FrozenCleanupCheck& operator=(FrozenCleanupCheck&& other) noexcept
+    {
+        should_freeze = other.should_freeze;
+        other.should_freeze = false;
+        return *this;
+    }
 };
 
 // Static Allocations
diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp
index f684e01e36..429570526f 100644
--- a/src/test/fuzz/checkqueue.cpp
+++ b/src/test/fuzz/checkqueue.cpp
@@ -13,7 +13,7 @@
 
 namespace {
 struct DumbCheck {
-    const bool result = false;
+    bool result = false;
 
     explicit DumbCheck(const bool _result) : result(_result)
     {

I ran a benchmark CCheckQueueSpeedPrevectorJob with merge-base, the PR, and the PR with my changes:

| ns/job | job/s | err% | ins/job | cyc/job | IPC | bra/job | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 395.62 | 2,527,646.43 | 0.7% | 225.93 | 400.00 | 0.565 | 45.40 | 7.6% | 5.53 | merge-base | 384.72 | 2,599,320.73 | 0.7% | 244.92 | 402.79 | 0.608 | 49.41 | 7.0% | 5.56 | PR | 364.97 | 2,739,962.15 | 0.7% | 213.05 | 377.85 | 0.564 | 44.54 | 7.7% | 5.48 | proposed changes

hebasto commented at 1:36 PM on March 20, 2023:

Thanks! Updated.

martinus approved

martinus commented at 7:49 PM on March 19, 2023: contributor

Code review ACK ff6c0abeea, ran the unit tests, commented performance nits with suggested changes

DrahtBot requested review from maflcko on Mar 19, 2023

DrahtBot requested review from ryanofsky on Mar 19, 2023

DrahtBot requested review from sedited on Mar 19, 2023

maflcko commented at 9:59 AM on March 20, 2023: member

re-ACK ff6c0abeea00deffcaeaf3b02092110d6895b1c0 🦀

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK ff6c0abeea00deffcaeaf3b02092110d6895b1c0 🦀
hZ7/6xv2f4dTKyfDPrNxYMnAswcLXyRz68Uxw+H6FsY8Vb1uva9MUm8EyMS0Xb0Vr/81mi0pfpPzkA94YgEEBw==

</details>

DrahtBot removed review request from maflcko on Mar 20, 2023

maflcko commented at 10:02 AM on March 20, 2023: member

nit in #26749 (review) is still unfixed?

hebasto force-pushed on Mar 20, 2023

hebasto commented at 1:35 PM on March 20, 2023: member

Updated ff6c0abeea00deffcaeaf3b02092110d6895b1c0 -> e580afee614b93236cb0e37457a5a7c28f4a0b73 (pr26749.11 -> pr26749.12, diff).

Addressed @martinus's performance nits:

The commit that fixes the use-after-free should come with a commit message to say that it fixed the bug

The "Fix "use-after-move" bug" commit has been separated. Sorry for misunderstanding you earlier.

in src/test/checkqueue_tests.cpp:189 in b8e35a2cd2 outdated

 187 |          size_t total = i;
 188 |          FakeCheckCheckCompletion::n_calls = 0;
 189 |          CCheckQueueControl<FakeCheckCheckCompletion> control(small_queue.get());
 190 |          while (total) {
 191 | -            vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
 192 | +            std::vector<FakeCheckCheckCompletion> vChecks{std::min(total, (size_t)InsecureRandRange(10))};

maflcko commented at 2:02 PM on March 20, 2023:

nit in b8e35a2: You can use std::min<size_t>(total, InsecureRandRange(.)) instead. Not that it matters, but this allows to check for unintentional unsigned integer wrap around, I think.

hebasto commented at 2:13 PM on March 20, 2023:

Thanks! Done.

hebasto force-pushed on Mar 20, 2023

hebasto commented at 2:12 PM on March 20, 2023: member

Updated e580afee614b93236cb0e37457a5a7c28f4a0b73 -> ebfc2a2dddedcbf384f435716d30d8418a77505b (pr26749.12 -> pr26749.13, diff):

addressed @MarcoFalke's comment

martinus commented at 6:50 PM on March 20, 2023: contributor

Code review ACK ebfc2a2dddedcbf384f435716d30d8418a77505b and ran git rebase --interactive upstream/master --exec 'make clean && make -j14 check'. Btw. I really links to the changes in your PR updates. Do you use any script to help with that?

DrahtBot requested review from maflcko on Mar 20, 2023

hebasto commented at 7:03 PM on March 20, 2023: member

@martinus

Btw. I really links to the changes in your PR updates. Do you use any script to help with that?

No, manual copy-paste :)

FWIW, I borrowed that idea from @ryanofsky.

sedited commented at 9:03 AM on March 21, 2023: contributor

re-ACK ebfc2a2dddedcbf384f435716d30d8418a77505b

DrahtBot removed review request from sedited on Mar 21, 2023

in src/validation.h:328 in b43ef231d2 outdated

 315 | @@ -316,17 +316,6 @@ class CScriptCheck
 316 |

maflcko commented at 10:53 AM on March 21, 2023:

in commit b43ef231d241e7f7ef7c7685a9ca7a6274d1b943

Any reason to keep the unused and dangerous CScriptCheck default constructor? (For example, it leaves txdata completely uninitialized?)

hebasto commented at 11:35 AM on March 21, 2023:

I can see no reasons to keep it. Removed.

maflcko commented at 10:07 AM on March 22, 2023:

Same commit: Can remove the now unused constructors for all other structs as well? See diff, plus unrelated cleanup: (Maybe for a follow-up to avoid another round of re-review?)

diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 012b0eb321..975dace633 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -57,7 +57,6 @@ struct FakeCheckCheckCompletion {
 struct FailingCheck {
     bool fails;
     FailingCheck(bool _fails) : fails(_fails){};
-    FailingCheck() : fails(true){};
     bool operator()() const
     {
         return !fails;
@@ -69,7 +68,6 @@ struct UniqueCheck {
     static std::unordered_multiset<size_t> results GUARDED_BY(m);
     size_t check_id;
     UniqueCheck(size_t check_id_in) : check_id(check_id_in){};
-    UniqueCheck() : check_id(0){};
     bool operator()()
     {
         LOCK(m);
@@ -86,7 +84,6 @@ struct MemoryCheck {
     {
         return true;
     }
-    MemoryCheck() = default;
     MemoryCheck(const MemoryCheck& x)
     {
         // We have to do this to make sure that destructor calls are paired
@@ -176,9 +173,7 @@ static void Correct_Queue_range(std::vector<size_t> range)
             control.Add(std::move(vChecks));
         }
         BOOST_REQUIRE(control.Wait());
-        if (FakeCheckCheckCompletion::n_calls != i) {
             BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i);
-        }
     }
     small_queue->StopWorkerThreads();
 }

hebasto commented at 10:12 AM on March 22, 2023:

Maybe for a follow-up to avoid another round of re-review?

Yes, please :)

fanquake commented at 10:57 AM on March 22, 2023:

Ok. We'll push this to a followup.

DrahtBot requested review from maflcko on Mar 21, 2023

maflcko commented at 10:53 AM on March 21, 2023: member

review ACK ebfc2a2dddedcbf384f435716d30d8418a77505b 🏡

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK ebfc2a2dddedcbf384f435716d30d8418a77505b 🏡
8gOAPf0pR4/+nDp5o9O6rQfe1M7luesZzUcTdpZRfLBu3Xf5FRCPgBX5My+qVdZ0cr94Q6nH645F0YyXQ2tRBg==

</details>

hebasto force-pushed on Mar 21, 2023

hebasto commented at 11:34 AM on March 21, 2023: member

Updated ebfc2a2dddedcbf384f435716d30d8418a77505b -> c7af97113459f50cf33882fa18909a6109e914f4 (pr26749.13 -> pr26749.14, diff).

Addressed @MarcoFalke's:

maflcko commented at 11:42 AM on March 21, 2023: member

Might be good to keep the clang-tidy related change in a separate commit, because it is independent (it is independent, because it can be applied on current master unreleated to the changes here in this pull)?

in src/test/checkqueue_tests.cpp:174 in c7af971134 outdated

 169 |          FakeCheckCheckCompletion::n_calls = 0;
 170 |          CCheckQueueControl<FakeCheckCheckCompletion> control(small_queue.get());
 171 |          while (total) {
 172 | -            vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
 173 | +            vChecks.clear();
 174 | +            vChecks.resize(std::min<size_t>(total, InsecureRandRange(vChecks.capacity() + 1)));

martinus commented at 12:22 PM on March 21, 2023:

I'm not sure that it is guaranteed that the capacity is actually the same as the one used for reserve(), as far as I know it can by anything that's at least as large as reserve(). E.g. power-of-two would be fine per the standard. I think with stdlibc++ it's actually 10, but no idea what e.g. Microsoft is doing. So I'd just leave this as InsecureRandRange(10)

martinus commented at 12:31 PM on March 21, 2023:

The standard doesn't seem to say anything else, so the real allocation capacity seems to be implementation defined: https://eel.is/c++draft/vector#capacity-4

hebasto commented at 1:08 PM on March 21, 2023:

Thanks! Updated.

hebasto force-pushed on Mar 21, 2023

hebasto commented at 12:26 PM on March 21, 2023: member

Updated c7af97113459f50cf33882fa18909a6109e914f4 -> a87002550c1461a4cfbe666d5034597276e32639 (pr26749.14 -> pr26749.15):

rebased to avoid a silent conflict with #26705
added a new commit "clang-tidy: Fix modernize-use-default-member-init in CScriptCheck" @MarcoFalke

Might be good to keep the clang-tidy related change in a separate commit, because it is independent (it is independent, because it can be applied on current master unreleated to the changes here in this pull)?

Like 06f5f3931910dcd2056167b1d440d327f3a510b0?

maflcko commented at 12:32 PM on March 21, 2023: member

Like https://github.com/bitcoin/bitcoin/commit/06f5f3931910dcd2056167b1d440d327f3a510b0?

I was referring to the bugprone-use-after-move workaround.

consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts` 15209d97c6

test, refactor: Avoid `CScriptCheck::swap` in `transaction_tests` 0682003214

refactor: Use move semantics in `CCheckQueue::Add`

Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>

6c2d5972f3

refactor: Make move semantics explicit for callers 04831fee6d

clang-tidy, test: Fix bugprone-use-after-move in `Correct_Queue_range()` 9a0b524139

refactor: Use move semantics in `CCheckQueue::Loop`

Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>

d8427cc28e

refactor: Drop no longer used `CScriptCheck()` default constructor b4bed5c1f9

clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck` a87fb6bee5

refactor: Drop no longer used `swap` member functions cea50521fe

test: Default initialize `should_freeze` to `true`

It is safe now, when move semantics is used instead of a custom swap
function.

95ad70ab65

hebasto force-pushed on Mar 21, 2023

hebasto commented at 1:08 PM on March 21, 2023: member

Updated a87002550c1461a4cfbe666d5034597276e32639 -> 95ad70ab652ddde7de65f633c36c1378b26a313a (pr26749.15 -> pr26749.16, diff):

addressed @martinus's comment
addressed @MarcoFalke's comment

martinus commented at 2:35 PM on March 21, 2023: contributor

re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a

DrahtBot requested review from maflcko on Mar 21, 2023

DrahtBot requested review from sedited on Mar 21, 2023

achow101 commented at 9:10 PM on March 21, 2023: member

ACK 95ad70ab652ddde7de65f633c36c1378b26a313a

sedited commented at 9:19 PM on March 21, 2023: contributor

re-ACK https://github.com/bitcoin/bitcoin/commit/95ad70ab652ddde7de65f633c36c1378b26a313a

DrahtBot removed review request from sedited on Mar 21, 2023

maflcko commented at 9:55 AM on March 22, 2023: member

re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥
3Jx5P5EQLnV2phsYvioBMSy53z+Oh8H+i3RnHRzSDyN/ear06Xcd0hZpgZIpjy2ToUbztFvgkSlnwG3PNzfGBQ==

</details>

DrahtBot removed review request from maflcko on Mar 22, 2023

fanquake merged this on Mar 22, 2023

fanquake closed this on Mar 22, 2023

hebasto deleted the branch on Mar 22, 2023

fanquake referenced this in commit 4c6b7d330a on Mar 22, 2023

sidhujag referenced this in commit bdfb91619e on Mar 23, 2023

sidhujag referenced this in commit 2e8d6d71d5 on Mar 23, 2023

bitcoin locked this on Mar 21, 2024

bitcoin unlocked this on Nov 29, 2024

in src/checkqueue.h:165 in 95ad70ab65

 161 | @@ -165,18 +162,15 @@ class CCheckQueue
 162 |      }
 163 |  
 164 |      //! Add a batch of checks to the queue
 165 | -    void Add(std::vector<T>& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
 166 | +    void Add(std::vector<T>&& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)

andrewtoth commented at 2:09 PM on November 29, 2024:

I don't think this change has any affect, aside from requiring std::move at the callsite.

In the method body we are only moving out of the individual elements, leaving a hollow element in its place. However, the actual vector vChecks is not moved so this would not have any change over using a regular lvalue reference.

This would be a benefit if somewhere in here we took ownership of vChecks, like

if (queue.empty()) {
    queue = std::move(vChecks);
}
```
Actually, even then it wouldn't matter because we can move out of the lvalue reference.

maflcko commented at 3:17 PM on November 29, 2024:

I don't think this change has any affect, aside from requiring std::move at the callsite.

Yes, and the benefits of requiring std::move are:

Self-documenting code, to mark an object as used.
use-after-move static analysis to catch violations, where a depleted object is re-used.

delta1 referenced this in commit e268f3a7eb on Apr 14, 2025

DashCoreAutoGuix referenced this in commit 026869dcdf on Jul 24, 2025

knst referenced this in commit 49c42f1a39 on Oct 22, 2025

bitcoin locked this on Nov 29, 2025