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
chore: remove some unnecessary calls to unwrap
/expect
#6727
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
pending @emhane this touches some net/transactions code ptal
3158b06
to
0ddf810
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed the tx pool and network crate, found one logic change which I don't think is in scope of this pr, see comment.
// Insert a new peer into the peerset. | ||
let peer = Peer::new(messages, version, client_version); | ||
let peer = match self.peers.entry(peer_id) { | ||
Entry::Occupied(mut entry) => { | ||
entry.insert(peer); | ||
entry.into_mut() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only logic change, to add the peer even if we are syncing, not sure what the motivation is behind this? found the call, my bad
@@ -350,7 +350,8 @@ where | |||
transaction: Self::Transaction, | |||
) -> PoolResult<TxHash> { | |||
let (_, tx) = self.validate(origin, transaction).await; | |||
self.pool.add_transactions(origin, std::iter::once(tx)).pop().expect("exists; qed") | |||
let mut results = self.pool.add_transactions(origin, std::iter::once(tx)); | |||
results.pop().expect("result length is the same as the input") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what this error message means, where are we guaranteed that transactions
ins't zero? anyhow out of scope of this pr probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_transaction takes in an iterator, the output is a Vec<Result<_>> collected from the iterator so it has the same length
} else { | ||
let jar = NippyJar::load(&self.path.join(segment.filename(block_range, tx_range))) | ||
.map(|jar| { | ||
let entry = match self.map.entry(key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this would acquire a lock which is not desirable
https://docs.rs/dashmap/latest/dashmap/struct.DashMap.html#method.entry
while get
allows this to be returned with reduced deadlock risk
https://docs.rs/dashmap/latest/dashmap/struct.DashMap.html#method.get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so does calling get twice no?
Mostly using
entry
andinsert*
APIs to avoid double lookups in maps andunwrap
/expect
in options.Also some cleanups along the way.