-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add address priority queue #210
feat: add address priority queue #210
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #210 +/- ##
==========================================
- Coverage 66.81% 65.72% -1.10%
==========================================
Files 23 23
Lines 901 916 +15
Branches 901 916 +15
==========================================
Hits 602 602
- Misses 273 288 +15
Partials 26 26 ☔ View full report in Codecov by Sentry. |
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.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware)
a discussion (no related file):
Let's discuss F2F if we need this PR.
Maybe the TransactionStore PR can replace this one.
crates/mempool/src/priority_queue.rs
line 65 at r1 (raw file):
New, Ignore, }
Why not use MempoolError
?
Code quote:
pub enum PriorityQueueTxResult {
Duplicate,
Replace(ThinTransaction),
New,
Ignore,
}
crates/mempool/src/priority_queue.rs
line 96 at r1 (raw file):
} for (index, tx) in self.0.iter_mut().enumerate() {
Do you need the for loop to catch the tx with the same nonce?
We assume there are no holes so that you can calculate the tx location in o(1).
Code quote:
for (index, tx) in self.0.iter_mut().enumerate()
crates/mempool/src/priority_queue.rs
line 103 at r1 (raw file):
let old_tx = self.0.remove(index); self.0.insert(index, new_tx); return PriorityQueueTxResult::Replace(old_tx);
I don't think we need to support transaction overriding at this MS
Code quote:
if new_tx.tip < tx.tip {
return PriorityQueueTxResult::Ignore;
}
let old_tx = self.0.remove(index);
self.0.insert(index, new_tx);
return PriorityQueueTxResult::Replace(old_tx);
3660514
to
b7c19c8
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.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @MohammadNassar1)
a discussion (no related file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Let's discuss F2F if we need this PR.
Maybe the TransactionStore PR can replace this one.
Done.
crates/mempool/src/priority_queue.rs
line 65 at r1 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Why not use
MempoolError
?
removed for now
crates/mempool/src/priority_queue.rs
line 96 at r1 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Do you need the for loop to catch the tx with the same nonce?
We assume there are no holes so that you can calculate the tx location in o(1).
removed for now
crates/mempool/src/priority_queue.rs
line 103 at r1 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
I don't think we need to support transaction overriding at this MS
removed for now
b7c19c8
to
6a23b8c
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.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)
crates/mempool/src/priority_queue.rs
line 76 at r3 (raw file):
pub fn top(&self) -> Option<ThinTransaction> { self.0.first().cloned() }
Suggestion:
pub fn top(&self) -> Option<&ThinTransaction> {
self.0.first()
}
crates/mempool/src/priority_queue.rs
line 79 at r3 (raw file):
pub fn pop(&mut self) -> Option<ThinTransaction> { Some(self.0.remove(0))
Suggestion:
self.0.pop()
6a23b8c
to
43f428f
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.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @MohammadNassar1)
crates/mempool/src/priority_queue.rs
line 76 at r3 (raw file):
pub fn top(&self) -> Option<ThinTransaction> { self.0.first().cloned() }
Done.
crates/mempool/src/priority_queue.rs
line 79 at r3 (raw file):
pub fn pop(&mut self) -> Option<ThinTransaction> { Some(self.0.remove(0))
pop Removes the last element from a vector and returns it, or [None
]
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 all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)
crates/mempool/src/priority_queue.rs
line 79 at r3 (raw file):
Previously, ayeletstarkware (Ayelet Zilber) wrote…
pop Removes the last element from a vector and returns it, or [
None
]
Got it!
Please rename to pop_front
crates/mempool/src/priority_queue.rs
line 78 at r4 (raw file):
} pub fn pop(&mut self) -> Option<ThinTransaction> {
When it returns None
?
Maybe you need to check first if it's empty.
Code quote:
Option<
43f428f
to
7b3446f
Compare
7b3446f
to
79c0696
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)
crates/mempool/src/priority_queue.rs
line 79 at r3 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Got it!
Please rename topop_front
Done.
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 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)
add address priority queue. Will be used in Mempool, multiple txs per account
This change is