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

[PART 1] Improve TX pool #1591

Closed
wants to merge 10 commits into from
Closed

[PART 1] Improve TX pool #1591

wants to merge 10 commits into from

Conversation

@evgenykuzyakov
Copy link
Collaborator

evgenykuzyakov commented Oct 29, 2019

Fixes 2 issues:

  • transactions with the same nonce, but for different access keys were filtered out before
  • transactions ordering was first by hash of a signer_id and then all transactions by the signer. Right now it fetches 1 transaction per (signer,public_key) pair first, then 2nd transaction per pair and so on to be more fair in tx ordering.

Switching from BTreeMap to Vec, since most (signer, pk) pairs will have exactly 1 transaction. Creating a BTreeMap for every transaction is a big overhead.

Fixes #1579

chain/pool/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

nearmax left a comment

Please don't forget to add a section on transaction ordering into https://github.com/nearprotocol/nomicon before submitting this PR.

chain/pool/src/lib.rs Outdated Show resolved Hide resolved
chain/pool/src/lib.rs Outdated Show resolved Hide resolved
self.transactions.remove(&key);
});
}
result.iter().for_each(|tx| {

This comment has been minimized.

Copy link
@nearmax

nearmax Oct 31, 2019

Collaborator

Use retain specialization.

This comment has been minimized.

Copy link
@nearmax

nearmax Oct 31, 2019

Collaborator

Also, above with the keys.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Nov 4, 2019

Author Collaborator

retain is slower, cause it iterates on the full map. This iterates only on the elements that needs to be removed.

let mut result = vec![];
while result.len() < expected_weight as usize && !self.transactions.is_empty() {
let mut keys_to_remove = vec![];
for (key, txs) in self.transactions.iter_mut() {

This comment has been minimized.

Copy link
@nearmax

nearmax Oct 31, 2019

Collaborator

Iteration over HashMap and HashSet is non-deterministic. Meaning, if we fill HashMap with kv-elements and iterate over keys or key-values the order will be different with different executions. This might not be an issue, since there is exactly one chunk producer, but I think we still want the order to be strictly defined not by some random seed that was used in initialization of HashSet, but by our random seed. Also, this seems to be a part the protocol, and we don't want to say the order of transaction in our protocol is defined through internal implementation of bucketing in HashMap.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Nov 4, 2019

Author Collaborator

As discussed offline decided this is intended behavior.
Adding docs to nomicon.

chain/pool/src/lib.rs Outdated Show resolved Hide resolved
@evgenykuzyakov evgenykuzyakov requested a review from nearmax Nov 4, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (staging@d56f351). Click here to learn what that means.
The diff coverage is 95.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##             staging    #1591   +/-   ##
==========================================
  Coverage           ?   79.79%           
==========================================
  Files              ?      130           
  Lines              ?    24041           
  Branches           ?        0           
==========================================
  Hits               ?    19184           
  Misses             ?     4857           
  Partials           ?        0
Impacted Files Coverage Δ
chain/chunks/src/lib.rs 93.94% <100%> (ø)
core/crypto/src/signature.rs 94.18% <66.66%> (ø)
chain/pool/src/lib.rs 97.48% <96.94%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d56f351...5faa4bb. Read the comment docs.

Copy link
Collaborator

nearmax left a comment

We also need the code that checks that transactions of the given block do not violate the ordering rule, and if it does slashes the block producer.

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

evgenykuzyakov commented Nov 5, 2019

We also need the code that checks that transactions of the given block do not violate the ordering rule, and if it does slashes the block producer.

Do you believe it should be the same PR? Cause there are no such challenge yet.

@evgenykuzyakov evgenykuzyakov changed the title Improve TX pool [PART 1] Improve TX pool Nov 6, 2019
@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

evgenykuzyakov commented Nov 6, 2019

I'll not push this separately. Going to just split it into 3 parts to simplify reviewing.

@nearmax
nearmax approved these changes Nov 6, 2019
Copy link
Collaborator

nearmax left a comment

Will this PR be submitted, now that we have 3 parts? I am approving it anyway.

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

evgenykuzyakov commented Nov 6, 2019

@nearmax My plan is to get approvals on all 3 parts, and merge 3rd part on top of staging instead.

pub transactions: HashMap<AccountId, BTreeMap<Nonce, SignedTransaction>>,
/// Transactions grouped by a pair of (account ID, signer public key).
/// It's more efficient to keep transactions unsorted and with potentially conflicting nonce
/// than create a BTreeMap for every transaction on average.

This comment has been minimized.

Copy link
@nearmax

nearmax Nov 11, 2019

Collaborator

The comment seems outdated. It is not BTreeMap anymore?

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Nov 13, 2019

Author Collaborator

We've previously used BTreeMap instead of Vec.

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

evgenykuzyakov commented Nov 13, 2019

Closing this, see #1657 for changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.