In the current mempool benchmarks, the coins were not being deleted from available_coins since Available rather than Available& was used. Also the lack of the reference meant that coin.vin_left++ only incremented the copy's variable rather than the one in the vector. The removal condition previously compared the coin's vin.size() to vin_left, when it should have been using vout.size(). I changed the MempoolCheck bench to instead use min_ancestors of 1 rather than 5 as otherwise an underflow could occur when calling coin.ref->vout.size() - coin.vin_left. This previously did not occur since vin_left was always 0 since a copy of the coin was used. Using 1 vs 5 shouldn't give too much of a difference since the test can still generate more than 1 (or 5) ancestors.
bench: remove from available_coins with reference, vout size #24975
pull Crypt-iQ wants to merge 1 commits into bitcoin:master from Crypt-iQ:pool_bench changing 1 files +2 −2-
Crypt-iQ commented at 1:08 PM on April 25, 2022: contributor
-
- DrahtBot added the label Tests on Apr 25, 2022
-
Crypt-iQ commented at 2:02 PM on April 25, 2022: contributor
With this change but without the
min_ancestors = 1change, the underflow would make my computer swap heavily and run out of disk space sincen_to_takewould be ~maxunsigned long -
DrahtBot commented at 5:44 PM on April 25, 2022: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
No conflicts as of last run.
-
Crypt-iQ commented at 12:20 PM on April 26, 2022: contributor
- DrahtBot added the label Needs rebase on Jun 2, 2022
- Crypt-iQ force-pushed on Jun 24, 2022
-
bench: remove from available_coins with reference, vout size 5fb9ba3971
- Crypt-iQ force-pushed on Jun 24, 2022
- DrahtBot removed the label Needs rebase on Jun 24, 2022
-
Crypt-iQ commented at 2:31 PM on June 24, 2022: contributor
Rebased since MempoolCheck no longer relies on the coin creation
-
-
Crypt-iQ commented at 4:22 PM on October 18, 2022: contributor
unfortunately don't have time to work on this, lmk if i should close it
-
glozow commented at 8:48 AM on October 22, 2022: member
Agree these are correct bug fixes. It's unclear to me why #22856, but not this, has the underflow problem?
unless changing the ComplexMempool bench should instead be using something like PopulateMempool with "real" coins?
This function and
PopulateMempoolare quite similar. IfPopulateMempoolcreates equally complex graphs of transactions, yeah, it probably makes sense to switch it over. -
achow101 commented at 5:50 PM on October 28, 2022: member
unfortunately don't have time to work on this, lmk if i should close it
Closing as Up For Grabs.
- achow101 closed this on Oct 28, 2022
- achow101 added the label Up for grabs on Oct 28, 2022
- bitcoin locked this on Oct 28, 2023
