Parametrize the transaction staging strategy & make it volatile by default #1131

dahlia · 2020-12-15T09:58:09Z

This patch parametrizes BlockChain<T>'s strategy to stage transactions through the new IStagePolicy<T> interface, and introduced VolatileStagePolicy<T>, which is an only built-in & default implementation.

Codecov Report

Merging #1131 (97263d7) into main (eb37cdf) will increase coverage by 0.08%.
The diff coverage is 97.50%.

@@            Coverage Diff             @@
##             main    #1131      +/-   ##
==========================================
+ Coverage   87.46%   87.55%   +0.08%     
==========================================
  Files         341      345       +4     
  Lines       29694    29956     +262     
==========================================
+ Hits        25972    26227     +255     
- Misses       2092     2097       +5     
- Partials     1630     1632       +2

Impacted Files	Coverage Δ
Libplanet/Net/Swarm.cs	`82.03% <50.00%> (+0.41%)`	⬆️
Libplanet/Net/Swarm.MessageHandlers.cs	`84.57% <71.42%> (-0.86%)`	⬇️
...ibplanet.Tests/Blockchain/BlockChainTest.Append.cs	`96.78% <96.78%> (ø)`
Libplanet/Blockchain/BlockChain.cs	`90.81% <97.87%> (-0.22%)`	⬇️
Libplanet.Tests/Blockchain/BlockChainTest.cs	`98.62% <97.91%> (+0.32%)`	⬆️
Libplanet.RocksDBStore.Tests/RocksDBStoreTest.cs	`88.23% <100.00%> (+0.35%)`	⬆️
...sts/Blockchain/BlockChainTest.ValidateNextBlock.cs	`94.91% <100.00%> (+0.04%)`	⬆️
...lanet.Tests/Blockchain/Policies/BlockPolicyTest.cs	`79.25% <100.00%> (+0.31%)`	⬆️
...lanet.Tests/Blockchain/Policies/StagePolicyTest.cs	`100.00% <100.00%> (ø)`
...sts/Blockchain/Policies/VolatileStagePolicyTest.cs	`100.00% <100.00%> (ø)`
... and 16 more

dahlia · 2020-12-18T08:54:53Z

@planetarium/libplanet PTAL.

longfin · 2020-12-18T09:11:33Z

Libplanet/Blockchain/BlockChain.cs

            {
-                _rwlock.ExitWriteLock();
+                StagePolicy.Stage(this, transaction);


Is StagePolicy.Stage() thread-safe?

It should be so (interface-wise), and I believe VolatileStagePolicy<T>, the only implementation, is so. (It uses ConcurrentDictionary<K, V> under the hood.)

OK, if .Unstage() is too, it seems that we can strip _rwlock in BlockChain<T>.UnstageTransaction().

longfin · 2020-12-18T09:13:02Z

Libplanet/Blockchain/BlockChain.cs

+                    .ToImmutableHashSet();
+                foreach (TxId txId in txIds)
+                {
+                    StagePolicy.Unstage(this, txId);


Is StagePolicy.Unstage() also thread-safe?

I made every operation on IStagePolicy<T> (protocol-wise) & VolatileStagePolicy<T> (implementation-wise) sure they are thread safe: 131ed7d.

longfin · 2020-12-18T09:16:21Z

Libplanet/Net/Swarm.MessageHandlers.cs

            {
+                Transaction<T> tx = BlockChain.GetTransaction(txid);
+
+                if (tx is null)


Can BlockChain<T>.GetTransaction() return null?

If txid is invalid there's the possibility.

[changelog skip] planetarium#1131 (comment)

dahlia · 2020-12-18T11:51:10Z

@planetarium/libplanet PTAL.

Libplanet/Blockchain/Policies/VolatileStagePolicy.cs

moreal · 2020-12-18T14:46:14Z

Libplanet/Blockchain/Policies/VolatileStagePolicy.cs

+        /// </summary>
+        public VolatileStagePolicy()
+        {
+            _set = new ConcurrentDictionary<TxId, Transaction<T>>();


It seems there is no code to delete _set's element. Is there a default capacity?

No, we need to implement lifetiming, but I'm going to do it on a different PR.

Libplanet/Blockchain/BlockChain.cs

longfin

There are comments about the style of the code left, but there seems to be no big problem with the behavior right now.

moreal

LGTM.

[changelog skip]

planetarium#1130

planetarium#1131 (comment)

[changelog skip]

[changelog skip] planetarium#1131 (comment)

planetarium#1131 (comment)

dahlia · 2020-12-22T04:50:31Z

@planetarium/libplanet I rebased on the current main. Please review this again!

dahlia self-assigned this Dec 15, 2020

dahlia force-pushed the stage-txs-on-memory branch from 324ac02 to 9d87645 Compare December 17, 2020 05:13

dahlia changed the title ~~WIP: Make staged transactions volatile and store them only on memory~~ Make staged transactions volatile and store them only on memory Dec 17, 2020

dahlia changed the title ~~Make staged transactions volatile and store them only on memory~~ Parametrize the transaction staging strategy & make it volatile by default Dec 17, 2020

dahlia marked this pull request as ready for review December 17, 2020 05:18

dahlia requested review from limebell, moreal, riemannulus and longfin December 17, 2020 05:18

longfin reviewed Dec 17, 2020

View reviewed changes

Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved

dahlia added a commit to dahlia/libplanet that referenced this pull request Dec 17, 2020

Make staged transactions not to immediately be put into store

a496a0e

planetarium#1131 (comment)

dahlia requested a review from longfin December 17, 2020 14:06

longfin previously approved these changes Dec 18, 2020

View reviewed changes

dahlia added a commit to dahlia/libplanet that referenced this pull request Dec 18, 2020

Guarantee all operations on IStagePolicy<T> are thread-safe

131ed7d

[changelog skip] planetarium#1131 (comment)

dahlia dismissed longfin’s stale review via 131ed7d December 18, 2020 11:49

dahlia requested a review from longfin December 18, 2020 11:51

longfin reviewed Dec 18, 2020

View reviewed changes

Libplanet/Blockchain/Policies/VolatileStagePolicy.cs Outdated Show resolved Hide resolved

moreal reviewed Dec 18, 2020

View reviewed changes

riemannulus reviewed Dec 21, 2020

View reviewed changes

Libplanet/Blockchain/BlockChain.cs Show resolved Hide resolved

longfin previously approved these changes Dec 21, 2020

View reviewed changes

moreal previously approved these changes Dec 21, 2020

View reviewed changes

riemannulus previously approved these changes Dec 22, 2020

View reviewed changes

dahlia added 5 commits December 22, 2020 13:29

Move BlockChainTest methods related to Append() to a new partial class

b889e3f

[changelog skip]

Make sure BlockChain<T>.Append() withdraw expired txs from stage

9563403

[changelog skip]

Parametrize the staging strategy & make it volatile by default

f99bad7

planetarium#1130

Make staged transactions not to immediately be put into store

4e859f2

planetarium#1131 (comment)

Polyfill SkipLast() for older .NET versions

c1fe1ef

dahlia added 3 commits December 22, 2020 13:31

Complete be free from PersistentStagePolicy<T>

359f256

[changelog skip]

Guarantee all operations on IStagePolicy<T> are thread-safe

73fab87

[changelog skip] planetarium#1131 (comment)

Simplify lock & release on VolatileStagePolicy<T>.Stage()

97263d7

planetarium#1131 (comment)

dahlia dismissed stale reviews from riemannulus, moreal, and longfin via 97263d7 December 22, 2020 04:49

dahlia force-pushed the stage-txs-on-memory branch from 131ed7d to 97263d7 Compare December 22, 2020 04:49

riemannulus approved these changes Dec 22, 2020

View reviewed changes

longfin self-requested a review December 22, 2020 04:55

longfin approved these changes Dec 22, 2020

View reviewed changes

moreal approved these changes Dec 22, 2020

View reviewed changes

limebell approved these changes Dec 22, 2020

View reviewed changes

dahlia merged commit 64b0d03 into planetarium:main Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parametrize the transaction staging strategy & make it volatile by default #1131

Parametrize the transaction staging strategy & make it volatile by default #1131

dahlia commented Dec 15, 2020 •

edited

Loading

codecov bot commented Dec 17, 2020 •

edited

Loading

dahlia commented Dec 18, 2020

longfin Dec 18, 2020

dahlia Dec 18, 2020

longfin Dec 18, 2020

longfin Dec 18, 2020

dahlia Dec 18, 2020

longfin Dec 18, 2020

dahlia Dec 18, 2020

longfin Dec 18, 2020

dahlia commented Dec 18, 2020

moreal Dec 18, 2020

dahlia Dec 18, 2020

longfin left a comment

moreal left a comment

dahlia commented Dec 22, 2020

Parametrize the transaction staging strategy & make it volatile by default #1131

Parametrize the transaction staging strategy & make it volatile by default #1131

Conversation

dahlia commented Dec 15, 2020 • edited Loading

codecov bot commented Dec 17, 2020 • edited Loading

Codecov Report

dahlia commented Dec 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dahlia commented Dec 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longfin left a comment

Choose a reason for hiding this comment

moreal left a comment

Choose a reason for hiding this comment

dahlia commented Dec 22, 2020

dahlia commented Dec 15, 2020 •

edited

Loading

codecov bot commented Dec 17, 2020 •

edited

Loading