feat(txpool) modify txpool guard to be for pipeline syncs only #4075

0xprames · 2023-08-05T10:07:57Z

should close #4066

the tests in network/src/transactions.rs need a bit of work - I think there's some bugs hiding in them rn.

0xprames · 2023-08-05T10:11:40Z

crates/net/network/src/transactions.rs

+        // assert!(!pool.is_empty()); this keeps failing - indicating that there's a bug with these
+        // tests
+
+        // if we attempt to modify the initial test above with SyncState::Idle, the same assert


cc @mattsse re these tests

tl;dr - assert pool.is_empty() and assert !pool.is_empty() don't seem to be doing what we want it to in these tests (?)

what do we want here?

not following

uncommented assert!(!pool.is_empty()); and added another test to highlight what I mean.

codecov · 2023-08-05T10:14:40Z

Codecov Report

Merging #4075 (dc6d636) into main (6622f53) will increase coverage by 0.06%.
The diff coverage is 86.14%.

Files Changed	Coverage Δ
crates/interfaces/src/sync.rs	`40.00% <0.00%> (-10.00%)`	⬇️
crates/net/network-api/src/lib.rs	`100.00% <ø> (ø)`
crates/net/network-api/src/noop.rs	`76.92% <0.00%> (-6.42%)`	⬇️
crates/net/network/src/transactions.rs	`61.39% <88.19%> (+11.73%)`	⬆️
crates/net/network/src/network.rs	`58.75% <100.00%> (+3.95%)`	⬆️

... and 13 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.85% <0.60%> (-0.04%)`	⬇️
unit-tests	`64.00% <86.14%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`25.40% <ø> (+0.11%)`	⬆️
blockchain tree	`82.83% <ø> (ø)`
pipeline	`90.07% <ø> (ø)`
storage (db)	`74.59% <ø> (ø)`
trie	`94.67% <ø> (-0.05%)`	⬇️
txpool	`47.66% <ø> (+0.07%)`	⬆️
networking	`77.67% <87.73%> (+0.23%)`	⬆️
rpc	`58.67% <ø> (-0.01%)`	⬇️
consensus	`63.76% <ø> (ø)`
revm	`32.26% <ø> (ø)`
payload builder	`6.57% <ø> (ø)`
primitives	`87.89% <0.00%> (-0.04%)`	⬇️

mattsse

cool, I first envisioned that the initial_sync_done flag will be tracked in the transactionmanager, but I think we can also just move it to the Syncstate impl, perhaps this is useful somewhere else.

ATM this seems perfectly reasonable but perhaps the txpool needs additional logic around this, but I can't think of anything else to check and just using the initial sync as the guard is sufficient atm.
so I'm supportive of adding it to syncstate.

tracking another atomic bool is inexpensive so this is fine, but we could put all of them in one atomicu8

mattsse · 2023-08-05T11:09:27Z

crates/net/network-api/src/lib.rs

@@ -49,6 +49,9 @@ pub trait NetworkInfo: Send + Sync {

    /// Returns `true` if the network is undergoing sync.
    fn is_syncing(&self) -> bool;
+
+    /// Returns `true` if the network is undergoing an initial sync.


This needs some additional context what this exactly represents wrt pipeline sync:
if this is a fresh node sync then this is expected to track the first complete pipeline run.

crates/net/network/src/network.rs

mattsse · 2023-08-05T11:38:23Z

crates/net/network/src/network.rs

+        let future_state = state.is_syncing();
+        let curr_state = self.inner.is_syncing.load(Ordering::Relaxed);
+        let init_sync_done = self.inner.initial_sync_done.load(Ordering::Relaxed);
+        self.inner.is_syncing.store(future_state, Ordering::Relaxed);
+        if !init_sync_done {
+            // we've either been in Idle or pipeline Syncing here, check to see if we're
+            // moving from a pipeline sync to idle
+            if curr_state && !future_state {
+                // set initial_sync_done flag to true here, every sync after this will be a livesync
+                self.inner.initial_sync_done.store(true, Ordering::Relaxed);
+            }
+        }


I think we can simplify this,
basically initial_sync_done will be true when is_syncing goes from false to true

I think we can use .swap here and compare the replaced value

mm let me think about this - I think I see what you're saying, but I need think through the implementation

I think you're right in that we know when is_syncing goes from false-> true (the first time, but is effectively "idempotent" every time after), initial_sync_done -> true

yeah I think this will replace the block that I have now:

self.inner.initial_sync_done.compare_exchange(false, curr_state && !future_state, Ordering::Relaxed, Ordering::Relaxed);

basically a CAS (compare_and_swap seems deprecated in favor of this new compare_exchange api) that only sets initial_sync_done if its false

I do want to mention that initial_sync_done should only be true when !init_sync_done && is_syncing goes from true->false

what you mentioned may not work (when is_syncing goes from false->true) because I think we need to capture when the initial_sync completes and not just when it starts.

otherwise in the network manager code we won't be able to differentiate between a live_sync and an ongoing pipeline sync (i.e is_syncing() && is_initial_syncing() will be ~~true~~ false**** even during the pipeline sync

what we want to capture is the state transition from false->true->false with is_syncing

false->true showcases that we've kicked off a pipeline sync, but we can't mark is_initial_done until it has actually completed

mattsse · 2023-08-07T10:51:26Z

crates/net/network-api/src/lib.rs

@@ -49,6 +49,9 @@ pub trait NetworkInfo: Send + Sync {

    /// Returns `true` if the network is undergoing sync.
    fn is_syncing(&self) -> bool;
+
+    /// Returns `true` when a fresh node is initially being synced, known as a Pipeline sync


this is not quite accurate because this also returns true on restart if the first pipeline sync is active.

It sounds like fresh node being initially synced is what's incorrect/misleading. let me reword this to say Returns true when a node is undergoing a pipeline sync

because during a pipeline sync, state of the sync "job" is stored externally I think, and it'll be loaded in/continued from upon restart

mattsse

good progress almost there

mattsse · 2023-08-07T12:19:08Z

crates/net/network/src/network.rs

+        let future_state = state.is_syncing();
+        let curr_syncing = self.inner.is_syncing.load(Ordering::Relaxed);
+        self.inner.is_syncing.store(future_state, Ordering::Relaxed);
+        // explicitly throw this error away since we expect it to error every time after it's been
+        // set to true, i.e after every live sync
+        let _ = self.inner.initial_sync_done.compare_exchange(
+            false,
+            // on first move back from Syncing->Idle - swap and set init_sync_done to true
+            curr_syncing && !future_state,
+            Ordering::Relaxed,
+            Ordering::Relaxed,
+        );


this is not quite exactly what I had in mind, you can do a swap on line 272 and then update is_initial_done depending on the values

ah.. ok yeah. I misunderstood the initial comment oops, but Ill keep the CAS bc it looks cleaner (and setting init_sync_done it in one atomic step/without a load and a store is nice)

basically what you're saying is instead of the load/store on is_syncing I just need one swap and the return value is what is being set as curr_syncing (which I'm going to rename to prev_state to make a bit clearer)

mattsse · 2023-08-08T09:16:41Z

crates/net/network/src/network.rs

+        let _ = self.inner.initial_sync_done.compare_exchange(
+            false,
+            // on first move back from Syncing->Idle - swap and set init_sync_done to true
+            prev_state && !future_state,
+            Ordering::Relaxed,
+            Ordering::Relaxed,
+        );


I really don't like that we don't handle the result,
I would prefer to update the initial_sync_done only if prev_state was false and future_state is true, this would be easier to read

mattsse · 2023-08-08T09:17:04Z

crates/net/network-api/src/lib.rs

@@ -49,6 +49,9 @@ pub trait NetworkInfo: Send + Sync {

    /// Returns `true` if the network is undergoing sync.
    fn is_syncing(&self) -> bool;
+
+    /// Returns `true` when a fresh node being bootstrapped is undergoing a Pipeline sync


Suggested change

/// Returns `true` when a fresh node being bootstrapped is undergoing a Pipeline sync

/// Returns `true` when the node is undergoing the very first Pipeline sync.

mattsse · 2023-08-08T09:17:42Z

crates/net/network/src/transactions.rs

+        // assert!(!pool.is_empty()); this keeps failing - indicating that there's a bug with these
+        // tests
+
+        // if we attempt to modify the initial test above with SyncState::Idle, the same assert


what do we want here?

not following

0xprames · 2023-08-08T14:47:17Z

what do we want here?
not following

@mattsse - I added broken tests to the latest commit to showcase the behavior im calling out.

mattsse

awesome,

I made two changes

also the failing tests test_tx_broadcasts_through_two_syncs can be fixed by mocking a session established, so there's a new peer in the set.

this is the last thing to do

mattsse · 2023-08-09T12:24:55Z

crates/net/network/src/network.rs

+        let init_sync_done = self.inner.initial_sync_done.load(Ordering::Relaxed);
+        let is_currently_syncing = self.inner.is_syncing.load(Ordering::Relaxed);
+        is_currently_syncing && !init_sync_done
+    }


I flattened the checks here, because we only need to load is_syncing if initial_sync_done is false

awesome makes sense, good catch

mattsse · 2023-08-09T12:27:06Z

crates/net/network/src/transactions.rs

+                    // we penalize the peer that sent the bad transaction with
+                    // the assumption that the peer should have known that this
+                    // transaction is bad. (e.g. consensus rules)
+                    if err.is_bad_transaction() && !this.network.is_initially_syncing() {


I changed this to currently syncing

yeah I think we need this for every sync, good catch thanks

…doc comment for flag

…itial_syncing flag

0xprames requested review from mattsse, Rjected and gakonst as code owners August 5, 2023 10:07

0xprames commented Aug 5, 2023

View reviewed changes

mattsse requested changes Aug 5, 2023

View reviewed changes

mattsse added the A-tx-pool Related to the transaction mempool label Aug 5, 2023

0xprames requested a review from mattsse August 5, 2023 23:21

0xprames force-pushed the txpool-new-guard branch from b0d7eca to d28e5a8 Compare August 6, 2023 00:13

mattsse reviewed Aug 7, 2023

View reviewed changes

mattsse requested changes Aug 7, 2023

View reviewed changes

0xprames force-pushed the txpool-new-guard branch from d28e5a8 to 1eca103 Compare August 7, 2023 14:44

0xprames requested a review from mattsse August 7, 2023 14:44

mattsse requested changes Aug 8, 2023

View reviewed changes

0xprames requested a review from mattsse August 8, 2023 14:47

mattsse requested changes Aug 9, 2023

View reviewed changes

0xprames and others added 9 commits August 9, 2023 10:58

feat(txpool) modify txpool guard to be for pipeline syncs only

da380a6

refactor/simplify init_sync_done logic, add pipeline sync context to …

0f01360

…doc comment for flag

remove unecessary ret statement, make linter happy

246ea67

simplify one more load/store into swap, more comment clarity on is_in…

2bf479f

…itial_syncing flag

review comments, showcase integ test failures w.r.t txpool

11fd700

break one more existing test to showcase hidden bug in tests

83ed55b

some touchups

9997c9e

add mock peer sessions/poll_fn to two_sync test, works

2f820cf

add poll/mock peer to existing ignore_while_syncing test

dc6d636

0xprames force-pushed the txpool-new-guard branch from af3c6de to dc6d636 Compare August 9, 2023 14:59

0xprames requested a review from mattsse August 9, 2023 15:00

mattsse approved these changes Aug 9, 2023

View reviewed changes

mattsse added this pull request to the merge queue Aug 9, 2023

Merged via the queue into paradigmxyz:main with commit fd7e28e Aug 9, 2023

mattsse mentioned this pull request Aug 14, 2023

Set eth_syncing to true if we failed to make canonical block on FCU due to missing block #4187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(txpool) modify txpool guard to be for pipeline syncs only #4075

feat(txpool) modify txpool guard to be for pipeline syncs only #4075

0xprames commented Aug 5, 2023

0xprames Aug 5, 2023

mattsse Aug 8, 2023

0xprames Aug 8, 2023

codecov bot commented Aug 5, 2023 •

edited

Loading

mattsse left a comment

mattsse Aug 5, 2023

mattsse Aug 5, 2023

0xprames Aug 5, 2023 •

edited

Loading

0xprames Aug 5, 2023 •

edited

Loading

0xprames Aug 5, 2023 •

edited

Loading

mattsse Aug 7, 2023

0xprames Aug 7, 2023

0xprames Aug 7, 2023

mattsse left a comment

mattsse Aug 7, 2023

0xprames Aug 7, 2023 •

edited

Loading

mattsse Aug 8, 2023 •

edited

Loading

0xprames Aug 8, 2023

mattsse Aug 8, 2023

0xprames Aug 8, 2023

mattsse Aug 8, 2023

0xprames commented Aug 8, 2023

mattsse left a comment

mattsse Aug 9, 2023

0xprames Aug 9, 2023

mattsse Aug 9, 2023

0xprames Aug 9, 2023

	/// Returns `true` when a fresh node being bootstrapped is undergoing a Pipeline sync
	/// Returns `true` when the node is undergoing the very first Pipeline sync.

feat(txpool) modify txpool guard to be for pipeline syncs only #4075

feat(txpool) modify txpool guard to be for pipeline syncs only #4075

Conversation

0xprames commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2023 • edited Loading

Codecov Report

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xprames Aug 5, 2023 • edited Loading

Choose a reason for hiding this comment

0xprames Aug 5, 2023 • edited Loading

Choose a reason for hiding this comment

0xprames Aug 5, 2023 • edited Loading

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

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xprames Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

mattsse Aug 8, 2023 • edited Loading

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

0xprames commented Aug 8, 2023

mattsse 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

codecov bot commented Aug 5, 2023 •

edited

Loading

0xprames Aug 5, 2023 •

edited

Loading

0xprames Aug 5, 2023 •

edited

Loading

0xprames Aug 5, 2023 •

edited

Loading

0xprames Aug 7, 2023 •

edited

Loading

mattsse Aug 8, 2023 •

edited

Loading