feat: dynamically adjust missing txs set #20300
Conversation
deffrian
force-pushed
the
nikita/dynamic-missing-txs
branch
from
5784054 to
47f62bc
Compare
deffrian
changed the title
deffrian
force-pushed
the
nikita/dynamic-missing-txs
branch
3 times, most recently
from
a90e1a6 to
47720b6
Compare
deffrian
force-pushed
the
nikita/dynamic-missing-txs
branch
from
47720b6 to
3d0e498
Compare
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
# Conflicts: # yarn-project/p2p/src/services/tx_collection/file_store_tx_collection.ts # yarn-project/p2p/src/services/tx_collection/slow_tx_collection.ts # yarn-project/p2p/src/services/tx_collection/tx_collection.ts # yarn-project/p2p/src/services/tx_collection/tx_collection_sink.ts
mrzeszutko
left a comment
mrzeszutko
left a comment
There was a problem hiding this comment.
I don't see part 2 from the linear issue (async generator) - I understand the MissingTxsTracker was supposed to solve the same issue in a different way but seems there some issues with how it is now implemented (please refer to fast_tx_collection.ts comment)
| try { | ||
| const buffer = await this.fileStore.read(path); | ||
| const tx = Tx.fromBuffer(buffer); | ||
| if ((await tx.validateTxHash()) && txHash === tx.txHash) { |
There was a problem hiding this comment.
Are you sure this === will work? won't it always be false as it checks reference equality for TxHash objects?
There was a problem hiding this comment.
this probably should be covered by a test as well
There was a problem hiding this comment.
It's covered by a test. Not sure but I think it compares by value
| const fastRequests = this.fastCollection.getFastCollectionRequests(); | ||
| const fastCollectionTxs: Set<string> = new Set( | ||
| ...Array.from(fastRequests.values()).flatMap(r => r.missingTxHashes), | ||
| ...Array.from(fastRequests.values()).flatMap(r => r.missingTxTracker.missingTxHashes), |
There was a problem hiding this comment.
Not introduced by your changes but there seems to be a preexisting bug in this line - can you check if ... works as expected? below is what claude said about this:
flatMap iterates each Set<string> and collects individual hash strings into an array. The ... spread then passes each string as a separate argument to new Set(). But the Set constructor only accepts a single iterable argument. So new Set("0x1abc...", "0x2def...") becomes new Set("0x1abc..."), which iterates the characters of that first hash string, producing Set {"0", "x", "1", "a", "b", "c", ...}.
The filter at line 204 (!fastCollectionTxs.has(txHash)) never matches a real tx hash, so slow collection runs even when those txs are already being fast-collected.
Fix: remove the spread — new Set(Array.from(fastRequests.values()).flatMap(r => [...r.missingTxTracker.missingTxHashes])).
| if (request.missingTxHashes.has(txHash)) { | ||
| request.missingTxHashes.delete(txHash); | ||
| request.foundTxs.set(txHash, tx); | ||
| if (request.missingTxTracker.markFetched(tx)) { |
There was a problem hiding this comment.
there seems to be an issue with this (found by claude but seems legit):
tIn fast_tx_collection.ts line 135, Promise.race([collectionPromise, request.promise.promise]) is meant to short-circuit collectFast as soon as all txs are found. However, request.promise.resolve() is only called from foundTxs (line 352-357) when request.missingTxTracker.markFetched(tx) returns true. It never returns true for collection-sourced txs because both the RPC path (line 239-241) and the reqresp path (MissingTxMetadataCollection.markFetched at missing_txs.ts line 186) call tracker.markFetched(tx) inside the sink callback, before the sink emits txs-added. By the time the foundTxs handler fires, the hash is already deleted from the tracker's set, so markFetched returns false and request.promise.resolve() is never reached.
The natural shouldStop() / notFinished() checks on the shared tracker do terminate the loops eventually, but collectFast must wait for Promise.allSettled([reqresp, nodeCollection]) to fully settle. With a working early-exit, it would return immediately when the last tx is found instead of waiting for in-flight reqresp workers to complete.
deffrian
changed the title
3 participants
Ref: A-489