This addresses a few review comments in #31363 after merge:
group_data to group_entry)GetConnectedComponent and use it in the txgraph fuzz test).FixLinearization)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32151.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs, glozow |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
1366@@ -1367,7 +1367,7 @@ void FixLinearization(const DepGraph<SetType>& depgraph, std::span<DepGraphIndex
1367 // in between forward.
1368 while (place_before.Any()) {
1369 // j cannot be 0 here; if it was, then there was necessarily nothing earlier which
1370- // elem needs to be place before anymore, and place_before would be empty.
1371+ // elem needs to be placed before anymore, and place_before would be empty.
If you want to fix typos in the touched files, there may be a bit more:
0[03:14:31.857] src/txgraph.cpp:342: Entrys ==> Entries, Entry
1[03:14:31.857] src/txgraph.cpp:415: Entrys ==> Entries, Entry
2[03:14:31.857] src/txgraph.cpp:512: Entrys ==> Entries, Entry
3[03:14:31.857] src/txgraph.cpp:1944: occured ==> occurred
4[03:14:31.857] src/txgraph.h:102: commiting ==> committing
249@@ -250,10 +250,7 @@ class DepGraph
250 return ret;
251 }
252
253- /** Find some connected component within the subset "todo" of this graph.
254- *
255- * Specifically, this finds the connected component which contains the first transaction of
256- * todo (if any).
257+ /** Get the connected component within the subset "todo" that contains tx.
tx must be in todo
460+ {
461+ uint64_t picked_num{0};
462+ try {
463+ reader >> VARINT(picked_num);
464+ } catch (const std::ios_base::failure&) {}
465+ if (picked_num < todo.Size() && todo[picked_num]) {
todo, and nothing seems to be breaking. Shouldn’t this cause an Assume failure when holes existed in the serialization and the chosen number selects that unused hole?
This instantly crashes the fuzz tests for me:
0- if (picked_num < todo.Size() && todo[picked_num]) {
1+ if (picked_num < todo.Size()/* && todo[picked_num]*/) {
2 picked = picked_num;
3 }
560@@ -561,36 +561,23 @@ FUZZ_TARGET(txgraph)
561 std::shuffle(refs.begin(), refs.end(), rng);
562 // Invoke the real function.
563 auto result = real->CountDistinctClusters(refs, use_main);
564- // Build a vector with representatives of the clusters the Refs occur in in the
565+ // Build a setr with representatives of the clusters the Refs occur in in the
This abstracts out the finding of the connected component that includes
a given element from FindConnectedComponent (which just finds any connected
component).
Use this in the txgraph fuzz test, which was effectively reimplementing this
logic. At the same time, improve its performance by replacing a vector with a
set.
ACK a52b53926b5c6a5b92255435e3c204cdf18665a2
The third comment was from me misremembering what FindConnectedComponent does, my bad. But I think it’s quite useful to have a GetConnectedComponent.