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

Patch/transaction ordering update #1326

Merged

Conversation

greymistcube
Copy link
Contributor

Closes #1322
Closes #1323

Ordering Transaction<T>s for evaluation has been separated from Block<T>() constructor as Block<T>.OrderTxsForEvaluation() static method. Additionally, in preparation for #1314, a new ordering algorithm has been introduced. New algorithm will kick in once ProtocalVersion gets increased to at least 2. Finally, pre-existing algorithm has been optimized for time and memory.

Here are some scripted benchmark results:

Number of Signers: 10
Number of Txs per Signer: 10
Running 1000 trials...

Old shuffling time: 2452 ticks
NewV0 shuffling time: 776 ticks
NewV2 shuffling time: 379 ticks

Number of Signers: 100
Number of Txs per Signer: 100
Running 1000 trials...

Old shuffling time: 190516 ticks
NewV0 shuffling time: 103822 ticks
NewV2 shuffling time: 26516 ticks

@greymistcube greymistcube added bug Something isn't working invalid This doesn't seem right labels Jun 2, 2021
@greymistcube greymistcube self-assigned this Jun 2, 2021
@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #1326 (c5a7652) into main (0033ed7) will decrease coverage by 9.10%.
The diff coverage is 53.94%.

❗ Current head c5a7652 differs from pull request most recent head 6e4d373. Consider uploading reports for the commit 6e4d373 to get more accurate results

@@            Coverage Diff             @@
##             main    #1326      +/-   ##
==========================================
- Coverage   86.03%   76.93%   -9.11%     
==========================================
  Files         361      253     -108     
  Lines       32457    17082   -15375     
==========================================
- Hits        27925    13142   -14783     
- Misses       2747     3410     +663     
+ Partials     1785      530    -1255     
Impacted Files Coverage Δ
...bplanet.Explorer/Controllers/ExplorerController.cs 0.00% <0.00%> (ø)
...rer/Controllers/GenericControllerNameConvention.cs 0.00% <0.00%> (ø)
Libplanet.Explorer/Controllers/GraphQLBody.cs 0.00% <0.00%> (ø)
Libplanet.Explorer/ExplorerStartup.cs 0.00% <0.00%> (ø)
Libplanet.Explorer/GraphTypes/NodeStateType.cs 0.00% <0.00%> (ø)
...ibplanet.Explorer/Interfaces/IBlockChainContext.cs 0.00% <0.00%> (ø)
Libplanet.Explorer/Queries/BlockQuery.cs 0.00% <0.00%> (ø)
Libplanet.Explorer/Queries/ExplorerQuery.cs 0.00% <0.00%> (ø)
Libplanet.Explorer/Queries/TransactionQuery.cs 0.00% <0.00%> (ø)
Libplanet.Explorer/Store/AddressRefDoc.cs 0.00% <0.00%> (ø)
... and 247 more

Copy link
Member

@limebell limebell left a comment

Choose a reason for hiding this comment

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

Can you please add a test case for this change? It would be great the added test fails at previous protocol, and pass at modified protocol.

And it would be great to have changelog describing this patch (in behavioral changes section).

@longfin
Copy link
Member

longfin commented Jun 5, 2021

/rebase

@libplanet libplanet force-pushed the patch/transaction-ordering-update branch from de98c7d to ed604d2 Compare June 5, 2021 06:43
@greymistcube greymistcube force-pushed the patch/transaction-ordering-update branch from ed604d2 to 025a55f Compare June 7, 2021 14:36
@greymistcube
Copy link
Contributor Author

@limebell

Can you please add a test case for this change?

See 89a0add. Hard coded spec is added to ensure that there is no behavioral change between the new optimized V0 implementation and the old implementation.

It would be great the added test fails at previous protocol, and pass at modified protocol.

Previous protocol doesn't "fail" per se, but does not work as intended. This is described in more detail in #1323. Statistical analysis may be done, but I don't see much point in doing so, and I'm not sure such test should be a part of unit testing. 😶

And it would be great to have changelog describing this patch (in behavioral changes section).

See 025a55f.

@greymistcube greymistcube requested a review from limebell June 8, 2021 04:50
@limebell
Copy link
Member

limebell commented Jun 9, 2021

/rebase

@libplanet libplanet force-pushed the patch/transaction-ordering-update branch from 868ee29 to 164c213 Compare June 9, 2021 01:36
limebell
limebell previously approved these changes Jun 9, 2021
@longfin
Copy link
Member

longfin commented Jun 21, 2021

@greymistcube it has a conflicting file...

@greymistcube greymistcube force-pushed the patch/transaction-ordering-update branch from 164c213 to 4bf485a Compare June 21, 2021 01:45
@greymistcube greymistcube removed the invalid This doesn't seem right label Jun 21, 2021
@greymistcube
Copy link
Contributor Author

@longfin Resolved.

@greymistcube greymistcube force-pushed the patch/transaction-ordering-update branch 2 times, most recently from 94930a1 to 2bf6773 Compare June 29, 2021 14:57
@limebell limebell self-requested a review July 1, 2021 05:13
@greymistcube greymistcube force-pushed the patch/transaction-ordering-update branch 2 times, most recently from eda233a to c5a7652 Compare July 6, 2021 02:47
@stale
Copy link

stale bot commented Jul 20, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The issue is stale label Jul 20, 2021
@limebell
Copy link
Member

not stale

@stale stale bot removed the stale The issue is stale label Jul 20, 2021
@greymistcube greymistcube force-pushed the patch/transaction-ordering-update branch from c5a7652 to 336dd7e Compare July 22, 2021 01:32
dahlia
dahlia previously approved these changes Jul 22, 2021
limebell
limebell previously approved these changes Jul 22, 2021
@greymistcube
Copy link
Contributor Author

/rebase

@libplanet libplanet dismissed stale reviews from limebell and dahlia via 6e4d373 July 22, 2021 08:25
@libplanet libplanet force-pushed the patch/transaction-ordering-update branch from 023544f to 6e4d373 Compare July 22, 2021 08:25
@limebell limebell requested review from dahlia and limebell July 22, 2021 08:28
@greymistcube greymistcube merged commit bebe479 into planetarium:main Jul 22, 2021
@greymistcube greymistcube deleted the patch/transaction-ordering-update branch July 22, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants