Skip to content

Conversation

sipa
Copy link
Member

@sipa sipa commented Aug 8, 2024

Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.

Split the FlushStateMode::ALWAYS mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in scantxoutset, gettxoutsetinfo, and in the currently-unused CreateUTXOSnapshot.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 8, 2024

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30610.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK andrewtoth, mzumsande

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:

  • #31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
  • #30611 (validation: write chainstate to disk every hour by andrewtoth)
  • #30595 (kernel: Introduce initial C header API by TheCharlatan)
  • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] 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.

@@ -6023,7 +6023,7 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
// returns in `ActivateSnapshot()`, when `MaybeRebalanceCaches()` is
// called, since we've added a snapshot chainstate and therefore will
// have to downsize the IBD chainstate, which will result in a call to
// `FlushStateToDisk(ALWAYS)`.
// `FlushStateToDisk(FORCE_FLUSH)`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear to me what the desired behavior is here, so I opted not to change it, but opinions welcome.

@andrewtoth
Copy link
Contributor

Concept ACK

Indeed, the FlushStateMode::ALWAYS variant is now confusing. It could mean we want to always write the chainstate to disk, or we want to always erase the dbcache. Splitting it up into the two cases makes sense.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 8, 2024

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/28531257299

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@l0rinc
Copy link
Contributor

l0rinc commented Aug 9, 2024

I understand this is still WIP, but checked quickly how much the current impl would speed up the IBD (first 500k blocks with prune, between 27a770b and 2a9c938).

Details
hyperfine \
--runs 3 \
--parameter-list COMMIT 27a770,2a9c93 \
--prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && ./autogen.sh && ./configure && make -j8 && rm -rf /mnt/sda1/BitcoinData/*' \
'COMMIT={COMMIT} ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=500000 -printtoconsole=0'
Benchmark 1: COMMIT=27a770 ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=500000 -printtoconsole=0
  Time (mean ± σ):     7999.675 s ± 63.767 s    [User: 9070.085 s, System: 750.952 s]
  Range (minmax):   7926.519 s8043.491 s    3 runs

Benchmark 2: COMMIT=2a9c93 ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=500000 -printtoconsole=0
  Time (mean ± σ):     9210.362 s ± 539.519 s    [User: 8944.738 s, System: 728.552 s]
  Range (minmax):   8713.643 s9784.349 s    3 runs
Summary
 'COMMIT=27a770 ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=500000 -printtoconsole=0' ran
   1.15 ± 0.07 times faster than 'COMMIT=2a9c93 ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=500000 -printtoconsole=0'

i.e. if the new impl seems consistently 15% slower than HEAD^1.

@sipa sipa force-pushed the 202408_force_sync branch from 2a9c938 to d61f2e5 Compare August 9, 2024 13:16
@sipa
Copy link
Member Author

sipa commented Aug 9, 2024

@paplorinc Huh, this PR shouldn't affect IBD speed at all.

EDIT: is this an IBD test from random peers? That will have way too much variance to conclude anything from 3 runs. I'd suggest a reindex test to avoid influence from randomly-selected peers on the internet.

@sipa sipa force-pushed the 202408_force_sync branch from d61f2e5 to 4471aaf Compare August 9, 2024 13:25
@andrewtoth
Copy link
Contributor

andrewtoth commented Aug 9, 2024

@paplorinc I don't see anything in this PR that would affect reindexing. This PR seems like a refactor only.
Oh, actually it will affectscantxoutset and gettxoutsetinfo rpcs.

@l0rinc
Copy link
Contributor

l0rinc commented Aug 12, 2024

Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.

I assumed (without diving into the code) that this meant an optimization.

Did 2x5 reindex checks until 500k blocks - 0% change.
  Time (mean ± σ):     15396.778 s ± 31.337 s    [User: 81183.230 s, System: 364.223 s]
  Range (min … max):   15363.334 s … 15427.586 s    5 runs

  Time (mean ± σ):     15410.168 s ± 20.832 s    [User: 81268.923 s, System: 363.370 s]
  Range (min … max):   15381.363 s … 15429.772 s    5 runs

Summary
  'COMMIT=f40365460247e3ce14dd59ca441fc243e679f228 ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -reindex -stopatheight=500000 -printtoconsole=0' ran
    1.00 ± 0.00 times faster than 'COMMIT=27a770b34b8f1dbb84760f442edb3e23a0c2420b ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -reindex -stopatheight=500000
-printtoconsole=0'

@sipa
Copy link
Member Author

sipa commented Aug 12, 2024

@paplorinc Yes, it is an optimization, but only affects the scantxoutset and gettxoutsetinfo RPCs. Normal synchronization should be unaffected.

@sipa sipa force-pushed the 202408_force_sync branch 3 times, most recently from 34a9cfd to 270371d Compare August 16, 2024 17:32
@sipa sipa force-pushed the 202408_force_sync branch from 270371d to 1ede480 Compare August 16, 2024 19:19
Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

The test interface_usdt_utxocache.py fails because generate() in wallet.py calls scantxoutset, which doesn't wipe the cache anymore. As a result, the number of utxos flushed to disk in later flushes changes.

Re: "currently-unused CreateUTXOSnapshot" in the PR description - it's used to create snapshots (dumptxoutset).

@achow101
Copy link
Member

cc @davidgumberg @andrewtoth @l0rinc

@l0rinc
Copy link
Contributor

l0rinc commented Nov 11, 2024

The test interface_usdt_utxocache.py fails

I haven't reviewed the code in detail yet (since the build is still failing).

reproduced build failure
> apt install systemtap-sdt-dev
> cmake -B build -DCMAKE_BUILD_TYPE=Release -DWITH_USDT=ON && cmake --build build -j$(nproc)
> build/test/functional/interface_usdt_utxocache.py

2024-11-11T15:47:49.211000Z TestFramework (INFO): handle_utxocache_add(): UTXOCacheChange(outpoint=d09ca39938b434b19a3338ef6b410071d85d4450dd7dc1f7db1bf7d95a53709a:0, height=201, value=4999968800sat, is_coinbase=False)
2024-11-11T15:47:49.211000Z TestFramework (INFO): handle_utxocache_spent(): UTXOCacheChange(outpoint=fab2396e3f726e1801b9e7303cf294783a3fd7e19ddc8f2dfa55e25b01575b3e:0, height=77, value=5000000000sat, is_coinbase=True)
2024-11-11T15:47:49.211000Z TestFramework (INFO): check that we successfully traced 2 adds and 1 spent
2024-11-11T15:47:49.229000Z TestFramework (INFO): test the utxocache:flush tracepoint API
2024-11-11T15:47:49.229000Z TestFramework (INFO): hook into the utxocache:flush tracepoint
2024-11-11T15:47:50.288000Z TestFramework (INFO): stop the node to flush the UTXO cache
2024-11-11T15:47:50.390000Z TestFramework (INFO): handle_utxocache_flush(): UTXOCacheFlush(duration=11670, mode=FORCE_FLUSH, size=3, memory=262464, for_prune=False)
Exception ignored on calling ctypes callback function: <function PerfEventArray._open_perf_buffer.<locals>.raw_cb_ at 0x76705f695c60>
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/bcc/table.py", line 991, in raw_cb_
    callback(cpu, data, size)
  File "/mnt/my_storage/bitcoin/build/test/functional/interface_usdt_utxocache.py", line 351, in handle_utxocache_flush
    expected_flushes.remove({
ValueError: list.remove(x): x not in list
2024-11-11T15:47:50.390000Z TestFramework (INFO): handle_utxocache_flush(): UTXOCacheFlush(duration=8205, mode=FORCE_FLUSH, size=0, memory=262240, for_prune=False)
2024-11-11T15:47:50.408000Z TestFramework (INFO): check that we don't expect additional flushes
2024-11-11T15:47:50.408000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/mnt/my_storage/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/mnt/my_storage/bitcoin/build/test/functional/interface_usdt_utxocache.py", line 151, in run_test
    self.test_flush()
  File "/mnt/my_storage/bitcoin/build/test/functional/interface_usdt_utxocache.py", line 380, in test_flush
    assert_equal(0, len(expected_flushes))
  File "/mnt/my_storage/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(0 == 1)
2024-11-11T15:47:50.460000Z TestFramework (INFO): Stopping nodes

it is an optimization, but only affects the scantxoutset and gettxoutsetinfo RPCs

I've measured the effect of this change on the mentioned RPCs with up-to-date tip - (edit) but based on the feedback I misunderstood the assignment
gettxoutsetinfo benchmarks
hyperfine \
--show-output \
--warmup 1 --runs 10 \
--export-json /mnt/gettxoutsetinfo-30610.json \
--parameter-list COMMIT 900b17239fb25750fd30b4af6e6c38096374b71f,226b6c7d6e69c1f032079c8bc5af5710d8e54efb \
--prepare 'killall -q bitcoind; git checkout {COMMIT} && git clean -fxd && git reset --hard && cmake -B build && cmake --build build -j$(nproc) && ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -daemon -connect=0 -rpcuser=benchmark -rpcpassword=benchmark && sleep 10' \
--cleanup './build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark stop && sleep 10 && killall -9 bitcoind || true' \
'COMMIT={COMMIT} ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData gettxoutsetinfo >/dev/null'
individual measurements
"command": "COMMIT=900b17239fb25750fd30b4af6e6c38096374b71f ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData gettxoutsetinfo >/dev/null",
"times": [ 194.38279658410002, 259.16496413609997, 209.65035661110002, 207.9064897821, 245.2373909041, 200.04822835410002, 227.2078635091, 192.6374527831, 160.2041179331, 141.44879581310002 ],

"command": "COMMIT=226b6c7d6e69c1f032079c8bc5af5710d8e54efb ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData gettxoutsetinfo >/dev/null",
"times": [ 138.6323105461, 138.80884732910002, 137.4915245871, 137.2168089111, 139.4260206461, 144.2785081071, 136.9866460371, 137.38376074410002, 138.23486413010002, 136.55915986710002 ],
Summary
  'COMMIT=226b6c7d6e69c1f032079c8bc5af5710d8e54efb ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData gettxoutsetinfo >/dev/null' ran
    1.47 ± 0.26 times faster than 'COMMIT=900b17239fb25750fd30b4af6e6c38096374b71f ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData gettxoutsetinfo >/dev/null'

scantxoutset benchmark
hyperfine \
--show-output \
--warmup 1 --runs 10 \
--export-json /mnt/scantxoutset-30610.json \
--parameter-list COMMIT 900b17239fb25750fd30b4af6e6c38096374b71f,226b6c7d6e69c1f032079c8bc5af5710d8e54efb \
--prepare 'killall -q bitcoind; git checkout {COMMIT} && git clean -fxd && git reset --hard && cmake -B build && cmake --build build -j$(nproc) && ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -daemon -connect=0 -rpcuser=benchmark -rpcpassword=benchmark && sleep 10' \
--cleanup './build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark stop && sleep 10 && killall -9 bitcoind || true' \
"COMMIT={COMMIT} ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData scantxoutset start '[\"addr(32ixEdVJWo3kmvJGMTZq5jAQVZZeuwnqzo)\"]' >/dev/null"
individual measurements
"command": "COMMIT=900b17239fb25750fd30b4af6e6c38096374b71f ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData scantxoutset start '[\"addr(32ixEdVJWo3kmvJGMTZq5jAQVZZeuwnqzo)\"]' >/dev/null",
"times": [ 62.82799487242, 60.64005598342, 60.99546379442, 60.31839828842, 64.55913160942, 63.55735657542, 62.07198356042, 62.17213781942, 63.035974236419996, 61.785472937419996 ],

"command": "COMMIT=226b6c7d6e69c1f032079c8bc5af5710d8e54efb ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData scantxoutset start '[\"addr(32ixEdVJWo3kmvJGMTZq5jAQVZZeuwnqzo)\"]' >/dev/null",
"times": [ 61.64325996342, 61.09686494542, 61.077064575419996, 61.28694446342, 60.94913395142, 61.24807362942, 61.204911535419996, 61.23621690842, 61.174668652419996, 61.04969869342 ],
Summary
  'COMMIT=226b6c7d6e69c1f032079c8bc5af5710d8e54efb ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData scantxoutset start '["addr(32ixEdVJWo3kmvJGMTZq5jAQVZZeuwnqzo)"]' >/dev/null' ran
    1.02 ± 0.02 times faster than 'COMMIT=900b17239fb25750fd30b4af6e6c38096374b71f ./build/src/bitcoin-cli -rpcuser=benchmark -rpcpassword=benchmark -datadir=/mnt/my_storage/BitcoinData scantxoutset start '["addr(32ixEdVJWo3kmvJGMTZq5jAQVZZeuwnqzo)"]' >/dev/null'

@andrewtoth
Copy link
Contributor

The tests need to be fixed per #30610 (review).

@l0rinc what this PR accomplishes is not wiping the dbcache if calling those RPCs. It should not affect performance of the RPCs, but performance of connecting new blocks after calling the RPCs.

Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not affect performance of the RPCs

It seems I have misunderstood multiple times what to benchmark here (IBD, reindex, RPC), so if there's anything that actually needs benchmarking, please describe it in more detail.

@@ -2869,7 +2869,7 @@ bool Chainstate::FlushStateToDisk(
// It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage.
bool fPeriodicFlush = mode == FlushStateMode::PERIODIC && nNow > m_last_flush + DATABASE_FLUSH_INTERVAL;
// Combine all conditions that result in a full cache flush.
fDoFullFlush = (mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune;
fDoFullFlush = (mode == FlushStateMode::FORCE_FLUSH) || (mode == FlushStateMode::FORCE_SYNC) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me this complex condition contains a repeated hidden sub-expression that's needed later as empty_cache.
Could we split this up to reduce duplication and simplify the expressions?

bool should_empty_cache = (mode == FlushStateMode::FORCE_FLUSH) || fCacheLarge || fCacheCritical;
bool fDoFullFlush = should_empty_cache || (mode == FlushStateMode::FORCE_SYNC) || fPeriodicFlush || fFlushForPrune;
...
// Flush the chainstate (which may refer to block index entries).
if (should_empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()) {
    return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to coin database."));
}
Patch
diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp	(revision 1ede4803f1f5dffa55644d908062b52bf71394e7)
+++ b/src/validation.cpp	(date 1731581531144)
@@ -2807,7 +2807,6 @@
     try {
     {
         bool fFlushForPrune = false;
-        bool fDoFullFlush = false;
 
         CoinsCacheSizeState cache_state = GetCoinsCacheSizeState();
         LOCK(m_blockman.cs_LastBlockFile);
@@ -2869,7 +2868,8 @@
         // It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage.
         bool fPeriodicFlush = mode == FlushStateMode::PERIODIC && nNow > m_last_flush + DATABASE_FLUSH_INTERVAL;
         // Combine all conditions that result in a full cache flush.
-        fDoFullFlush = (mode == FlushStateMode::FORCE_FLUSH) || (mode == FlushStateMode::FORCE_SYNC) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune;
+        bool should_empty_cache = (mode == FlushStateMode::FORCE_FLUSH) || fCacheLarge || fCacheCritical;
+        bool fDoFullFlush = should_empty_cache || (mode == FlushStateMode::FORCE_SYNC) || fPeriodicFlush || fFlushForPrune;
         // Write blocks and block index to disk.
         if (fDoFullFlush || fPeriodicWrite) {
             // Ensure we can write block index
@@ -2917,8 +2917,7 @@
                 return FatalError(m_chainman.GetNotifications(), state, _("Disk space is too low!"));
             }
             // Flush the chainstate (which may refer to block index entries).
-            const auto empty_cache{(mode == FlushStateMode::FORCE_FLUSH) || fCacheLarge || fCacheCritical};
-            if (empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()) {
+            if (should_empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()) {
                 return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to coin database."));
             }
             m_last_flush = nNow;

@@ -448,7 +448,8 @@ enum class FlushStateMode {
NONE,
IF_NEEDED,
PERIODIC,
ALWAYS
FORCE_SYNC,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should likely update the related script and docs as well:

Patch
diff --git a/doc/tracing.md b/doc/tracing.md
--- a/doc/tracing.md	(revision 1ede4803f1f5dffa55644d908062b52bf71394e7)
+++ b/doc/tracing.md	(date 1731581879407)
@@ -126,8 +126,8 @@

 Arguments passed:
 1. Time it took to flush the cache microseconds as `int64`
-2. Flush state mode as `uint32`. It's an enumerator class with values `0`
-   (`NONE`), `1` (`IF_NEEDED`), `2` (`PERIODIC`), `3` (`ALWAYS`)
+2. Flush state mode as `uint32`. It's an enumerator class with values
+   `0` (`NONE`), `1` (`IF_NEEDED`), `2` (`PERIODIC`), `3` (`FORCE_SYNC`), `4` (`FORCE_FLUSH`)
 3. Cache size (number of coins) before the flush as `uint64`
 4. Cache memory usage in bytes as `uint64`
 5. If pruning caused the flush as `bool`

diff --git a/contrib/tracing/README.md b/contrib/tracing/README.md
--- a/contrib/tracing/README.md	(revision 1ede4803f1f5dffa55644d908062b52bf71394e7)
+++ b/contrib/tracing/README.md	(date 1731581127426)
@@ -247,10 +246,10 @@

 Logging utxocache flushes. Ctrl-C to end...
-Duration (µs)   Mode       Coins Count     Memory Usage    Prune
-730451          IF_NEEDED  22990           3323.54 kB      True
-637657          ALWAYS     122320          17124.80 kB     False
-81349           ALWAYS     0               1383.49 kB      False
+Duration (µs)   Mode        Coins Count     Memory Usage    Prune
+730451          IF_NEEDED   22990           3323.54 kB      True
+637657          FORCE_FLUSH 122320          17124.80 kB     False
+81349           FORCE_FLUSH 0               1383.49 kB      False


diff --git a/contrib/tracing/log_utxocache_flush.py b/contrib/tracing/log_utxocache_flush.py
--- a/contrib/tracing/log_utxocache_flush.py	(revision 1ede4803f1f5dffa55644d908062b52bf71394e7)
+++ b/contrib/tracing/log_utxocache_flush.py	(date 1731582100621)
@@ -45,7 +45,8 @@
     'NONE',
     'IF_NEEDED',
     'PERIODIC',
-    'ALWAYS'
+    'FORCE_SYNC',
+    'FORCE_FLUSH',
 ]

@@ -2941,10 +2941,10 @@ bool Chainstate::FlushStateToDisk(
return true;
}

void Chainstate::ForceFlushStateToDisk()
void Chainstate::ForceFlushStateToDisk(bool wipe_cache)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that ForceFlushStateToDisk(false) does a FORCE_SYNC now, does the method name still reflect its role?

@@ -678,7 +679,7 @@ class Chainstate
int nManualPruneHeight = 0);

//! Unconditionally flush all changes to disk.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here, is Unconditionally still the case here, given that ForceFlushStateToDisk(false) does a sync instead?

@@ -368,8 +369,8 @@ def handle_utxocache_flush(_, data, __):
# A node shutdown causes two flushes. One that flushes UTXOS_IN_CACHE
Copy link
Contributor

@l0rinc l0rinc Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to UTXOS_IN_CACHE = 3 and expected_flushes.append({"mode": "NONE", "for_prune": True, "size": BLOCKS_TO_MINE}) makes this test pass again.

patch
diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py
--- a/test/functional/interface_usdt_utxocache.py	(revision eb1257260b2f4ede605241f1fb514452d63305c8)
+++ b/test/functional/interface_usdt_utxocache.py	(date 1731587669301)
@@ -365,7 +365,7 @@
         bpf["utxocache_flush"].open_perf_buffer(handle_utxocache_flush)
 
         self.log.info("stop the node to flush the UTXO cache")
-        UTXOS_IN_CACHE = 2 # might need to be changed if the earlier tests are modified
+        UTXOS_IN_CACHE = 3 # might need to be changed if the earlier tests are modified
         # A node shutdown causes two flushes. One that flushes UTXOS_IN_CACHE
         # UTXOs and one that flushes 0 UTXOs. Normally the 0-UTXO-flush is the
         # second flush, however it can happen that the order changes.
@@ -395,7 +395,7 @@
         bpf["utxocache_flush"].open_perf_buffer(handle_utxocache_flush)
 
         self.log.info(f"prune blockchain to trigger a flush for pruning")
-        expected_flushes.append({"mode": "NONE", "for_prune": True, "size": 0})
+        expected_flushes.append({"mode": "NONE", "for_prune": True, "size": BLOCKS_TO_MINE})
         self.nodes[0].pruneblockchain(315)
 
         bpf.perf_buffer_poll(timeout=500)

@andrewtoth
Copy link
Contributor

@l0rinc a way to test that this PR is working properly is to first run master and sync a few blocks. In the UpdateTip logs you will see a cache= value that increases. Running either of these RPCs should make the next UpdateTip reset a few MB at most.
When doing the same test on this branch, the next UpdateTip value should be the same or higher than the previous.

@DrahtBot
Copy link
Contributor

Needs rebase, if still relevant

@DrahtBot
Copy link
Contributor

Converted to draft for now, due to inactivity

@DrahtBot
Copy link
Contributor

DrahtBot commented May 1, 2025

🐙 This pull request conflicts with the target branch and needs rebase.

@DrahtBot
Copy link
Contributor

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants