OrderTxsForEvaluationV3 added #1518

greymistcube · 2021-10-11T16:52:44Z

Related to #1326.
Closes #1322.
Closes #1323.

This tries to readdress the issues mentioned in #1322 and #1323. While working on this PR, initial attempt was made to implement Fisher-Yates shuffling, but it turns out this is no trivial task.

First, some context on why Fisher-Yates is not used in this PR. The goal is to use PreEvaluationHash of a Block<T> to deterministically pseudo-randomly shuffle transactions by signer addresses. What Fisher-Yates algorithm provides is an unbiased sampling, but this requires higher entropy than that of PreEvaluationHash can provide by itself.¹ Proper implementation would require increasing entropy by extending PreEvaluationHash with each transaction's signature, not signer address, on each step of selection required for Fisher-Yates. As my own implementation got more and more elaborate, I became concerned about possible performance degradation, so decided to take another route.

Implementation in this PR tries to achieve at least the following.

Given n signers, the probability of any signer to be selected as first is 1/n.
Given any pair of signers a and b, the probability of a being selected before b is 1/2.

Keep in mind, these conditions are not sufficient enough to give us unbiased permutation samplings of n signers, which is a potential security hole, however minor it may be. As two "consecutive" signers in the original ordering is highly likely to be executed consecutively, although not necessarily in order. This can be somewhat mitigated easily with some additional computation, but cannot be completely normalized while using only PreEvaluationHash as the random seed.

1: For unbiased sampling of permutations of n elements, minimum required entropy is log2(n!), meaning approximately 524 randomly generated bits are required for shuffling 100 elements.

codecov · 2021-10-11T17:15:20Z

Codecov Report

Merging #1518 (8160415) into main (9214167) will decrease coverage by 57.77%.
The diff coverage is 0.00%.

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

@@             Coverage Diff             @@
##             main    #1518       +/-   ##
===========================================
- Coverage   84.65%   26.88%   -57.78%     
===========================================
  Files         262      276       +14     
  Lines        9240    18116     +8876     
===========================================
- Hits         7822     4870     -2952     
- Misses       1418    12856    +11438     
- Partials        0      390      +390

Impacted Files	Coverage Δ
Libplanet/Action/ActionEvaluator.cs	`43.54% <0.00%> (-50.63%)`	⬇️
Libplanet/Net/IceServer.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/Net/PeerState.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/Net/BlockDemand.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/Net/Messages/Tx.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/Net/SwarmOptions.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/Net/Messages/Ping.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/Net/Messages/Pong.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/Net/Messages/TxIds.cs	`0.00% <0.00%> (-100.00%)`	⬇️
Libplanet/TimeSpanExtensions.cs	`0.00% <0.00%> (-100.00%)`	⬇️
... and 271 more

greymistcube · 2021-10-13T02:24:38Z

Just some benchmark result

Method	NumSigners	NumTxsPerSigner	ProtocolVersion	Mean	Error	StdDev
OrderTxs	64	8	0	419.9 ms	3.72 ms	2.90 ms
OrderTxs	64	8	3	274.9 ms	5.22 ms	5.36 ms

Performance improvement seems to be about 30%.

stale · 2021-10-30T06:05:30Z

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 · 2021-11-16T11:52:04Z

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.

riemannulus · 2021-11-25T04:20:14Z

Libplanet/Action/ActionEvaluator.cs

@@ -590,6 +592,43 @@ ActionContext CreateActionContext(IAccountStateDelta prevStates, int randomSeed)
                .SelectMany(group => group.OrderBy(tx => tx.Nonce));
        }

+        [Pure]
+        private static IEnumerable<Transaction<T>> OrderTxsForEvaluationV3(


We don't have OrderTxsForEvaluationV3 test here. Could you add some tests for this method?

Actually, OrderTxsForEvaluation() in ActionEvaluatorTest.cs covers all cases. 🙃

greymistcube · 2021-11-25T12:29:39Z

Bumped Block<T>.CurrentProtocolVersion from 2 to 3.

Libplanet.Tests/Action/ActionEvaluatorTest.cs

CHANGES.md

Tests integrated Changelog Typo Clarity Remove changelog

More optimization Edge cases

Rephrasing for clarity

Missing empty line Internal class

pull-request-quantifier-deprecated · 2022-02-22T02:02:55Z

This PR has 172 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +112 -60
Percentile : 54.4%

Total files changed: 7

Change summary by file extension:
.md : +7 -2
.cs : +105 -58

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

greymistcube force-pushed the feature/tx-shuffle branch from 4967433 to 6677943 Compare October 11, 2021 16:53

greymistcube marked this pull request as ready for review October 11, 2021 23:53

auto-assign bot requested review from dahlia, kfangw, limebell and longfin October 11, 2021 23:53

auto-assign bot assigned greymistcube Oct 11, 2021

greymistcube force-pushed the feature/tx-shuffle branch 2 times, most recently from 23d5c21 to bf0004b Compare October 12, 2021 00:13

greymistcube force-pushed the feature/tx-shuffle branch from ce5916b to 8160415 Compare October 13, 2021 02:27

stale bot added the stale The issue is stale label Oct 30, 2021

greymistcube removed the stale The issue is stale label Nov 2, 2021

stale bot added the stale The issue is stale label Nov 16, 2021

riemannulus reviewed Nov 25, 2021

View reviewed changes

stale bot removed stale The issue is stale labels Nov 25, 2021

greymistcube force-pushed the feature/tx-shuffle branch 2 times, most recently from f9feecf to 2d1c363 Compare November 25, 2021 12:21

greymistcube force-pushed the feature/tx-shuffle branch 3 times, most recently from 7a66323 to 8dba46d Compare November 29, 2021 01:26

kfangw removed their request for review February 7, 2022 07:29

greymistcube force-pushed the feature/tx-shuffle branch from 8dba46d to ba463ab Compare February 21, 2022 05:01

pull-request-quantifier-deprecated bot added the Medium label Feb 21, 2022

greymistcube force-pushed the feature/tx-shuffle branch 3 times, most recently from ff4dd90 to a25fd2a Compare February 21, 2022 06:39

dahlia reviewed Feb 21, 2022

View reviewed changes

Libplanet.Tests/Action/ActionEvaluatorTest.cs Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

greymistcube force-pushed the feature/tx-shuffle branch from a25fd2a to fa8fc96 Compare February 21, 2022 12:41

greymistcube requested a review from dahlia February 21, 2022 12:53

greymistcube force-pushed the feature/tx-shuffle branch from 8af5067 to d8d370f Compare February 21, 2022 13:33

dahlia previously approved these changes Feb 22, 2022

View reviewed changes

greymistcube added 6 commits February 22, 2022 11:02

OrderTxsForEvaluationV3 added

789f89c

Tests integrated Changelog Typo Clarity Remove changelog

Slight optimization; test fix

c09afc3

More optimization Edge cases

CurrentProtocolVersion bumped

c4f1198

Changelog

47d3a9f

Rephrasing for clarity

Use theory for test

c271505

Missing empty line Internal class

Test fix

f62487e

greymistcube dismissed dahlia’s stale review via f62487e February 22, 2022 02:02

greymistcube force-pushed the feature/tx-shuffle branch from 7a739fb to f62487e Compare February 22, 2022 02:02

dahlia approved these changes Feb 22, 2022

View reviewed changes

dahlia merged commit d1b113d into planetarium:main Feb 22, 2022

greymistcube deleted the feature/tx-shuffle branch March 4, 2022 10:02

fufunoyu mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade node-fetch from 2.6.7 to 3.2.10 fufunoyu/libplanet#4

Open

checkoss mentioned this pull request Aug 1, 2022

[Snyk] Security upgrade node-fetch from 2.6.7 to 3.2.10 checkoss/libplanet#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OrderTxsForEvaluationV3 added #1518

OrderTxsForEvaluationV3 added #1518

greymistcube commented Oct 11, 2021 •

edited

Loading

codecov bot commented Oct 11, 2021 •

edited

Loading

greymistcube commented Oct 13, 2021 •

edited

Loading

stale bot commented Oct 30, 2021

stale bot commented Nov 16, 2021

riemannulus Nov 25, 2021

greymistcube Nov 25, 2021

greymistcube commented Nov 25, 2021 •

edited

Loading

pull-request-quantifier-deprecated bot commented Feb 22, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

OrderTxsForEvaluationV3 added #1518

OrderTxsForEvaluationV3 added #1518

Conversation

greymistcube commented Oct 11, 2021 • edited Loading

codecov bot commented Oct 11, 2021 • edited Loading

Codecov Report

greymistcube commented Oct 13, 2021 • edited Loading

stale bot commented Oct 30, 2021

stale bot commented Nov 16, 2021

riemannulus Nov 25, 2021

Choose a reason for hiding this comment

greymistcube Nov 25, 2021

Choose a reason for hiding this comment

greymistcube commented Nov 25, 2021 • edited Loading

pull-request-quantifier-deprecated bot commented Feb 22, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

greymistcube commented Oct 11, 2021 •

edited

Loading

codecov bot commented Oct 11, 2021 •

edited

Loading

greymistcube commented Oct 13, 2021 •

edited

Loading

greymistcube commented Nov 25, 2021 •

edited

Loading