fuzz: increase `txorphan` harness stability

marcofleon commented at 5:11 pm on May 28, 2024: contributor

This moves nNextSweep from being a static variable in LimitOrphans to being a member of the TxOrphanage class. This improves the stability of the txorphan fuzz harness, as each orphanage (created every iteration) now has its own value for nNextSweep.

DrahtBot commented at 5:11 pm on May 28, 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	dergoegge, maflcko, glozow

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:

#30111 (locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip by glozow)
#30110 (refactor: TxDownloadManager by glozow)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

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.

DrahtBot added the label Tests on May 28, 2024

in src/txorphanage.h:110 in c5af5a51a8 outdated

106@@ -107,7 +107,7 @@ class TxOrphanage {
107     int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
108 
109     /** Timestamp for the next scheduled sweep of expired orphans */
110-    NodeSeconds nNextSweep;
111+    NodeSeconds n_next_sweep;

dergoegge commented at 6:20 pm on May 28, 2024:

Should be m_ not n_, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

dergoegge commented at 6:29 pm on May 28, 2024: member

I suspect that the scripted diff is failing in CI but passing on your Mac because sed behaves differently on Mac vs Linux. Maybe this helps?

maflcko commented at 6:35 pm on May 28, 2024: member

but passing on your Mac

The script shouldn’t be passing on macos. Otherwise, https://github.com/bitcoin/bitcoin/commit/b3122e167a023df18f67c4083bc72f78968874db doesn’t work as intended and should be fixed.

DrahtBot added the label CI failed on May 28, 2024

marcofleon force-pushed on May 28, 2024

marcofleon commented at 11:51 pm on May 28, 2024: contributor

Seems like CI still doesn’t like it.

I tried this (no space after -i) sed -i'' 's/nNextSweep/m_next_sweep/g' $(git grep -l 'nNextSweep')

and it didn’t work on Mac. Adding the -e without .bak creates new txorphanage files with -e appended to the end of them. Something like txorphanage.h-e. Adding .bak prevented that but CI isn’t accepting it.

maflcko commented at 6:27 am on May 29, 2024: member

and it didn’t work on Mac.

You’ll have to install and use gnu-sed on macos, or use Linux. If you don’t want to test locally and want to know the syntax without testing, you can look at previous examples via git log --grep='sed -i'.

Though, I am not sure if a three line diff is worth a scripted diff, so an alternative would be to just squash everything down into a single commit.

in src/txorphanage.h:110 in 17d94a19df outdated

104@@ -104,6 +105,9 @@ class TxOrphanage {
105 
106     /** Erase an orphan by wtxid */
107     int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
108+
109+    /** Timestamp for the next scheduled sweep of expired orphans */
110+    NodeSeconds m_next_sweep GUARDED_BY(m_mutex);

maflcko commented at 6:38 am on May 29, 2024:

In any case, you are changing the code to leave the variable uninitialized. This is undefined behavior.

You can test this with valgrind, or msan.

marcofleon commented at 9:15 am on May 30, 2024:

Got it, thanks. Should be fixed now.

dergoegge commented at 8:10 am on May 29, 2024: member

I am not sure if a three line diff is worth a scripted diff

Just for context, I suggested it to marco as a learning exercise.

maflcko commented at 8:13 am on May 29, 2024: member

Sure, sounds good either way. Happy to review once CI is green and the UB is removed.

DrahtBot added the label Needs rebase on May 29, 2024

increase txorphan harness stability

initialize variable

0048680467

scripted-diff: Replace nNextSweep with m_next_sweep

-BEGIN VERIFY SCRIPT-
sed -i 's/nNextSweep/m_next_sweep/g' $(git grep -l 'nNextSweep')
-END VERIFY SCRIPT-

fixing to match style

8defc182a3

marcofleon force-pushed on May 29, 2024

marcofleon marked this as ready for review on May 29, 2024

DrahtBot removed the label Needs rebase on May 29, 2024

DrahtBot removed the label CI failed on May 30, 2024

marcofleon commented at 9:18 am on May 30, 2024: contributor

You’ll have to install and use gnu-sed on macos, or use Linux. If you don’t want to test locally and want to know the syntax without testing, you can look at previous examples via git log --grep='sed -i'.

I installed gnu-sed and used that locally. CI looks good now.

dergoegge approved

dergoegge commented at 9:38 am on May 30, 2024: member

Code review ACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09

maflcko commented at 10:52 am on May 30, 2024: member

utACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09

maflcko commented at 10:53 am on May 30, 2024: member

cc @glozow This conflicts with some of your pulls, so maybe you can have a look here? (https://github.com/bitcoin/bitcoin/pull/30186#issuecomment-2135744973)

glozow commented at 6:52 am on June 2, 2024: member

utACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09, I can rebase on this pretty easily

fanquake merged this on Jun 3, 2024

fanquake closed this on Jun 3, 2024

fuzz: increase txorphan harness stability #30186

Code Coverage

Reviews

Conflicts

fuzz: increase `txorphan` harness stability #30186