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.

Sign up for GitHub

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

Jump to bottom

Fix transaction route #2116

Closed
wants to merge 12 commits into from
Closed

Fix transaction route #2116

wants to merge 12 commits into from

Conversation

Qiao-Jin
Copy link
Contributor

@Qiao-Jin Qiao-Jin commented Dec 3, 2020

Close #2115

@erikzhang
Copy link
Member

We need a unified inventory message stream. If the original stream does not have a great impact on TPS, I think there is no need to modify it.

@shargon
Copy link
Member

shargon commented Dec 7, 2020

@Qiao-Jin Do you have some benchmarks?

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Dec 8, 2020 •
edited
Loading

Attached is a comparison between TPS of this PR and master branch.

Env: 1 consensus node with 6 i7 cores and 16 GB rams

image

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Dec 9, 2020 •
edited
Loading

@erikzhang @shargon Could you have a check about the result?

@erikzhang
Copy link
Member

I think they are not much different.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Dec 9, 2020 •
edited
Loading

I think they are not much different.

Not so much but still significant optimization I think.

Besides, the effect of this optimization is limited, as current bottom neck is Blockchain receiving transactions. This is because we have included persisiting period into the 15 s block period in #1960. This means that Blockchain will have no time to receive transactions if traffic is heavy. If without this limitation the effect will be more significant.

Moreover the logic Blockchain receiving transaction array from MemoryPool is wrong, and is corrected in this PR.

@shargon
Copy link
Member

shargon commented Dec 9, 2020

If the benchmarks was after the fix (12c333e) this PR it's good to me

@Qiao-Jin
Copy link
Contributor Author

Ping

shargon
shargon approved these changes Dec 10, 2020
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good for me, wait for @erikzhang

erikzhang
erikzhang requested changes Dec 11, 2020
View reviewed changes
src/neo/Ledger/Blockchain.cs
case Transaction[] transactions:
// This message comes from a mempool's revalidation, already relayed
foreach (var tx in transactions) OnTransaction(tx, false);
foreach (var tx in transactions) OnNewTransaction(tx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The policy changed. You need to re-verify the transactions. And if you revert this change, tps will be reduced to the same as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose Func OnNewTransaction will call func VerifyStateDependent and policy is checked there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, if the policy was changed, the transactions needed to be completely re-verified, including VerifyStateIndependent. I'm not sure if it is unnecessary now.

Copy link
Contributor Author

@Qiao-Jin Qiao-Jin Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VerifyStateIndependent will check only the witness correctness now. Its price will be calculated in VerifyDependent together with others like transfer output, etc. Besides I observed that func MaxVerificationGas is untouched in #2045. Is this correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix this in another pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2107 should have solved this problem

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2107 moves TxRouter to NeoSystem too. Can you create a pr for fixing OnNewTransaction only?

@Qiao-Jin Qiao-Jin mentioned this pull request Dec 14, 2020
Merged
@Qiao-Jin Qiao-Jin mentioned this pull request Mar 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikzhang erikzhang erikzhang requested changes

@shargon shargon shargon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix transaction route
3 participants
@Qiao-Jin @erikzhang @shargon