Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

versioned TxHash wrapper, type safety in TransactionFetcher #6148

Closed
emhane opened this issue Jan 21, 2024 · 2 comments · Fixed by #6318
Closed

versioned TxHash wrapper, type safety in TransactionFetcher #6148

emhane opened this issue Jan 21, 2024 · 2 comments · Fixed by #6318
Labels
A-networking Related to networking in general C-enhancement New feature or request

Comments

@emhane
Copy link
Member

emhane commented Jan 21, 2024

Describe the feature

TransactionFetcher is a complex type. buffered_hashes is a subset of keys in unknown_hashes. keys in eth68_meta is also a subset of unknown_hashes. naturally one would like to aggregate those three into a type. nonetheless, inflight_requests and buffered_hashes are also an invariant, they are disjoint sets.

what would improve the type safety in TransactionFetcher most of all to start with, is making the separation between TxHashes from NewPooledTransactionHashes66 announcements and from NewPooledTransactionHashes68 announcements. this is useful since requests are packed differently for each version.

since we store both hash versions alongside each other in unknown_hashes and buffered_hashes, eventually we would probably wrap them in an enum, so it makes sense to make one versioned wrapper type to start with: NewTxHash that has a method eth_version.

'new' refers to 'unknown' + 'previously unseen upon arrival in an announcement' (check the on_new_pooled_transaction_hashes pipeline, then the latter makes more sense). a hash which makes it past this filter is a NewTxHash.

we could even have intermediary type UnknownTxHash, as output of the first filter, to make it clear that the input to filter_unseen_hashes is the type UnknownTxHash.

conversion from a TxHash to a versioned wrapper needs to be parametrised with the EthVersion. the best way to do this is to modify methods of NewPooledTransactionHashes to return the NewTxHash type that wraps type B256.

NewTxHash could optionally hold a (type, size) metadata tuple. this would mean that we don't need to store the size for eth68 hashes separately in eth68_meta. if we want to do this is debatable. if we do, then the versioned hash wrapper is a bit bigger than a TxHash. if we don't do this every eth68 TxHash in unknown_hashes is stored double up, since we need to store it in eth68_meta.

Additional context

No response

@emhane emhane added the S-needs-triage This issue needs to be labelled label Jan 21, 2024
@emhane emhane added the A-networking Related to networking in general label Jan 21, 2024
@DaniPopes DaniPopes added C-enhancement New feature or request and removed S-needs-triage This issue needs to be labelled labels Jan 22, 2024
@emhane
Copy link
Member Author

emhane commented Jan 22, 2024

on second thought, would prefer to store eth68 and eth66 separately, so two separate types NewTxHash68 and NewTxHash66. then we would have buffered_hashes_68 and buffered_hashes_66, and unknown_hashes_68 and unknown_hashes_66.

/// Hashes that are awaiting fetch from an idle peer.
pub(super) buffered_hashes: LruCache<TxHash>,
/// Tracks all hashes that are currently being fetched or are buffered, mapping them to
/// request retries and last recently seen fallback peers (max one request try for any peer).
pub(super) unknown_hashes: LruMap<TxHash, (u8, LruCache<PeerId>), Unlimited>,

@emhane
Copy link
Member Author

emhane commented Feb 7, 2024

#6427 implicitly closes this issue

@emhane emhane closed this as completed Feb 7, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Reth Tracker Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants