Fix possible incomplete txpool restore from dump file #7991

fab-10 · 2024-12-05T12:02:09Z

PR description

it could happen, especially at startup, that the node switch from in sync to out of sync quickly, and if that happens during the txpool restore from the dump file, the restore could be incomplete.

This is an log example of this issue:

1. Loading transaction pool content from file /data/besu/txpool.dump
2. Node out of sync, disabling transaction handling
3. Added 88 transactions of 442 loaded from file /data/besu/txpool.dump
4. Saving 88 transactions to file /data/besu/txpool.dump
5. Saved 88 transactions to file /data/besu/txpool.dump
6. Node is in sync, enabling transaction handling
7. Loading transaction pool content from file /data/besu/txpool.dump
8. Added 87 transactions of 88 loaded from file /data/besu/txpool.dump

So at the beginning the node was restoring the txpool (1.),
but while this restore was still in progress, the node went out of sync and the txpool was disabled(2.),
the issue manifest itself at this moment, since with the txpool disabled, restore txs are just ignored but technically the restore was completed successfully and the dump file removed, with only a subset of the txs restored (3.),
last part of disabling the txpool, consist in saving its content to the dump file (4.),
so the dump file is created with the current content of the txpool, that only has the subset of txs loaded from point 3 (5.)
Then after the node get in sync again, the process restart, but this time the dump file contains only a subset of the original txs set (6., 7., 8.)

To fix this issue, when disabling the txpool, we make sure that any in progress restore operation is cancelled correctly, removing from the dump file only the txs that have been restored, and changing the save operation to append to an existing dump file, so that no save txs are lost in case of a partial restore.

Below the new log with the fix

1. Loading transaction pool content from file /data/besu/txpool.dump
2. Node out of sync, disabling transaction handling
3. Added 94 transactions of 94 loaded from file /data/besu/txpool.dump, before operation was cancelled
4. Appending 94 transactions to file /data/besu/txpool.dump
5. Appended 94 transactions to file /data/besu/txpool.dump
6. Node is in sync, enabling transaction handling
7. Loading transaction pool content from file /data/besu/txpool.dump
8. Added 248 transactions of 348 loaded from file /data/besu/txpool.dump, deleting file

Note that restore operation is correctly stopped when the node goes out of sync (3.),
the save operation appends to the partially restored dump file (4., 5.)
and when the node goes in sync again the restore operation read the full set of txs from the dump, and deleted the file at the end (8.)

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests

daniellehrner · 2024-12-05T13:55:55Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

+        LOG.debug("Cancelling in progress read operation");
+        isCancelled.set(true);
+        try {
+          readInProgress.get().get();


It is not very clear in the code why this line is cancelling the read operation. Maybe it's worth it to put this call into a method with a descriptive name

readInProgress.get().get() wait for the completable future to finish, introduced a method to describe its meaning

pinges

Is there a reason why you don't just keep the current dump if the restore is canceled?

fab-10 · 2024-12-06T08:49:02Z

Is there a reason why you don't just keep the current dump if the restore is canceled?

The current dump is kept, but already restored txs are removed from it, to avoid duplicates.

In detail when a restore is cancelled, a subset of txs is added to the pool, along with other txs that have arrived via api/p2p, so when the restore is cancelled because the txpool is disabled, the restored txs are removed from the dump file, that is not deleted, and then the txpool is saved to disk, and in this operation the already restored and the new txs are appended to the existing dump file without duplications.

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

macfarla

maybe a changelog entry

Matilda-Clerke

Can't really comment on the meaningfulness of the changes, but LGTM, with some nits about var usage

Matilda-Clerke · 2024-12-17T23:17:24Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

@@ -645,15 +646,16 @@ public CompletableFuture<Void> setDisabled() {
      isPoolEnabled.set(false);
      subscribeConnectId.ifPresent(ethContext.getEthPeers()::unsubscribeConnect);
      pendingTransactionsListenersProxy.unsubscribe();
-      final PendingTransactions pendingTransactionsToSave = pendingTransactions;
+      final var saveOperation =


Would prefer to see a concrete type here rather than var. It's not entirely clear what type will be used.

done, I always hope the variable name is talkative enough to avoid the verbosity

Matilda-Clerke · 2024-12-17T23:26:15Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

@@ -839,41 +864,77 @@ private void executeLoadFromDisk() {
          LOG.info("Loading transaction pool content from file {}", saveFile);
          try (final BufferedReader br =
              new BufferedReader(new FileReader(saveFile, StandardCharsets.US_ASCII))) {
-            final IntSummaryStatistics stats =
+            final var stats =


Would prefer to see the concrete type here rather than var as it's not clear what the type would be

macfarla

LGTM

macfarla · 2024-12-19T02:59:17Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

-              } catch (ExecutionException ee) {
-                // nothing to do
-              }
-            }

            isCancelled.set(false);


does this need to be an atomic boolean still?

I guess yes because it's still used elsewhere

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 self-assigned this Dec 5, 2024

fab-10 marked this pull request as ready for review December 5, 2024 13:36

daniellehrner reviewed Dec 5, 2024

View reviewed changes

fab-10 force-pushed the fix-restoring-txpool-from-dump branch from fb3556d to ec5f053 Compare December 5, 2024 14:41

pinges reviewed Dec 5, 2024

View reviewed changes

macfarla reviewed Dec 9, 2024

View reviewed changes

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java Outdated Show resolved Hide resolved

macfarla reviewed Dec 9, 2024

View reviewed changes

fab-10 force-pushed the fix-restoring-txpool-from-dump branch from df85577 to 78867d7 Compare December 9, 2024 11:22

fab-10 force-pushed the fix-restoring-txpool-from-dump branch from 78867d7 to 03e28e7 Compare December 17, 2024 13:28

Matilda-Clerke reviewed Dec 17, 2024

View reviewed changes

fab-10 force-pushed the fix-restoring-txpool-from-dump branch from 03e28e7 to c04fd4a Compare December 18, 2024 08:58

fab-10 requested review from Matilda-Clerke, daniellehrner, macfarla and pinges December 18, 2024 08:59

macfarla approved these changes Dec 19, 2024

View reviewed changes

Fix possible incomplete txpool restore from dump file

9f6a23b

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the fix-restoring-txpool-from-dump branch from c04fd4a to 9f6a23b Compare December 19, 2024 10:55

fab-10 enabled auto-merge (squash) December 19, 2024 10:56

fab-10 merged commit ec79c17 into hyperledger:main Dec 19, 2024
43 checks passed

garyschulte pushed a commit that referenced this pull request Dec 20, 2024

Fix possible incomplete txpool restore from dump file (#7991)

979493f

Signed-off-by: Fabio Di Fabio <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix possible incomplete txpool restore from dump file #7991

Fix possible incomplete txpool restore from dump file #7991

fab-10 commented Dec 5, 2024 •

edited

Loading

daniellehrner Dec 5, 2024

fab-10 Dec 5, 2024

pinges left a comment

fab-10 commented Dec 6, 2024

macfarla left a comment

Matilda-Clerke left a comment

Matilda-Clerke Dec 17, 2024

fab-10 Dec 18, 2024

Matilda-Clerke Dec 17, 2024

fab-10 Dec 18, 2024

macfarla left a comment

macfarla Dec 19, 2024

macfarla Dec 19, 2024

Fix possible incomplete txpool restore from dump file #7991

Fix possible incomplete txpool restore from dump file #7991

Conversation

fab-10 commented Dec 5, 2024 • edited Loading

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinges left a comment

Choose a reason for hiding this comment

fab-10 commented Dec 6, 2024

macfarla left a comment

Choose a reason for hiding this comment

Matilda-Clerke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macfarla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fab-10 commented Dec 5, 2024 •

edited

Loading