Trusted state completer #946

dahlia · 2020-08-06T10:32:20Z

This patch is continued from the previous patch #934 and addresses the idea sketched in the issue #929.

However, in general I became to believe this approach is not appropriate. Here I elaborate:

Rendering and unrendering whole actions between two tips on reorg anyway requires all states of every step, and it means the idea of cherry-picking only recent states is pointless for this case.
Even if we decide to fetch (trust) all states from peers for every step between two tips on reorg, it rather takes longer time than recalculating all states.
We are slowly moving towards chain-interior-states (from chain-exterior-states, the status quo): Inserting a state hash into the BlockHeader #931, PreEvaluationHash on Block<T> and BlockHeader #935, and Introduce Merkle Patricia Trie #939. Eventually, we won't need any trust for reusing states calculated by peers, after all.

Hence, although I prepared this patch for a quite long time, I doubt this works well for the problem addressed by the issue #929. 🤔

codecov · 2020-08-06T10:53:55Z

Codecov Report

Merging #946 into main will increase coverage by 0.14%.
The diff coverage is 88.85%.

@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
+ Coverage   87.98%   88.13%   +0.14%     
==========================================
  Files         269      276       +7     
  Lines       24872    25270     +398     
==========================================
+ Hits        21883    22271     +388     
- Misses       1531     1540       +9     
- Partials     1458     1459       +1

Impacted Files	Coverage Δ
Libplanet.Tests/TestUtils.cs	`88.39% <0.00%> (-0.06%)`	⬇️
...lanet/Blockchain/IncompleteBlockStatesException.cs	`37.50% <0.00%> (ø)`
Libplanet/Blocks/Block.cs	`85.61% <ø> (ø)`
Libplanet/Net/Swarm.TrustedStateCompleter.cs	`66.25% <66.25%> (ø)`
Libplanet/Net/Swarm.MessageHandlers.cs	`85.71% <85.71%> (ø)`
Libplanet/Blockchain/BlockChain.cs	`90.98% <88.23%> (+0.82%)`	⬆️
Libplanet/Net/Swarm.cs	`86.26% <94.44%> (+0.20%)`	⬆️
Libplanet.Tests/Blockchain/BlockChainTest.cs	`98.36% <97.61%> (-0.03%)`	⬇️
Libplanet.Tests/Net/Messages/BlockStatesTest.cs	`100.00% <100.00%> (ø)`
Libplanet.Tests/Net/SwarmTest.Fixtures.cs	`100.00% <100.00%> (ø)`
... and 21 more

[changelog skip]

dahlia · 2020-08-12T05:11:09Z

@planetarium/libplanet Please review this.

Libplanet.Tests/Net/SwarmTest.TrustedStateCompleter.cs

longfin · 2020-08-12T06:35:15Z

Libplanet/Net/Swarm.TrustedStateCompleter.cs

+                            continue;
+                        }
+
+                        BlockChain.Store.SetBlockStates(blockStates.BlockHash, blockStates.States);


Is it fine without any of the mutex?

Rather it does not work (gets dead-locked) if we try to lock the BlockChain._rwlock here, because BlockChain<T>.GetState()/GetBalance() methods guarantee their stateCompleter to be called only in the read lock:

libplanet/Libplanet/Blockchain/BlockChain.cs

Lines 1680 to 1702 in ed9b1f8

_rwlock.EnterUpgradeableReadLock();

try

{

stateReference = Store.LookupStateReference(Id, key, block);

if (stateReference is null)

{

return null;

}

HashDigest<SHA256> hashValue = stateReference.Item1;

IImmutableDictionary<string, IValue> blockStates = Store.GetBlockStates(hashValue);

if (blockStates is null)

{

return rawStateCompleter(this, hashValue);

}

return blockStates.TryGetValue(key, out IValue state) ? state : null;

}

finally

{

_rwlock.ExitUpgradeableReadLock();

}

I see... then it seems that we should upgrade _rwlock before calling rawStateCompleter in BlockChain<T>.GetRawState().

Not all state completers update the blockchain, but only Recalculate completers do. Those implementations have already entered the write lock by themselves.

libplanet/Libplanet/Blockchain/BlockChain.cs

Lines 1638 to 1647 in ed9b1f8

_rwlock.EnterWriteLock();

try

{

SetStates(block, evaluations, buildStateReferences: false);

}

finally

{

_rwlock.ExitWriteLock();

}

Then, shouldn't these state completers upgrade _rwlocks? (like Recalculate)

@longfin I'm going to address this problem in the next separated pull request!

longfin · 2020-08-12T06:51:27Z

Libplanet/Net/Swarm.TrustedStateCompleter.cs

+            {
+                StateCompleter = (blockChain, blockHash, address) =>
+                {
+                    FillTrustedBlockStates(blockHash).Wait(cancellationToken);


It can be a trouble if StateCompleter is called in an async method... but it doesn't seem translatable to async because StateCompleter and related features are all synchronous. 🤔

That's what I'd grappled with, but I gave up and decided to request for comments about this problem…

@longfin I'm going to address this problem in the next separated pull request!

Libplanet.Tests/TestUtils.cs

Libplanet/Net/Swarm.TrustedStateCompleter.cs

Libplanet/Blockchain/BlockChain.cs

Co-authored-by: Seunghun Lee <waydi1@gmail.com>

planetarium#946 (comment) planetarium#946 (comment)

planetarium#946 (comment)

dahlia · 2020-08-13T02:26:08Z

@planetarium/libplanet Please review this again, folks!

planetarium#946 (comment)

planetarium#946 (comment) [changelog skip]

dahlia added network Related to networking (Libplanet.Net) storage Related to storage (Libplanet.Store) labels Aug 6, 2020

dahlia self-assigned this Aug 6, 2020

dahlia force-pushed the trusted-state-completer branch 2 times, most recently from 5186eac to 6354245 Compare August 8, 2020 07:05

riemannulus changed the base branch from master to main August 10, 2020 04:25

dahlia changed the title ~~WIP: Trusted state completer~~ Trusted state completer Aug 10, 2020

dahlia marked this pull request as ready for review August 10, 2020 07:39

dahlia requested review from earlbread, limebell, longfin, moreal and riemannulus August 10, 2020 07:39

dahlia added 4 commits August 11, 2020 20:55

More detailed explanation on dialTimeout parameters

da0adb4

[changelog skip]

Add append option to BlockChain<T>.MineBlock() method

be22e97

RandomExtensions for easier test writing

8e12da3

GetBlockStates and BlockStates messages

e512c08

dahlia force-pushed the trusted-state-completer branch 2 times, most recently from 772b1f6 to ed9b1f8 Compare August 11, 2020 15:17

longfin reviewed Aug 12, 2020

View reviewed changes

riemannulus reviewed Aug 12, 2020

View reviewed changes

Libplanet.Tests/TestUtils.cs Show resolved Hide resolved

earlbread reviewed Aug 12, 2020

View reviewed changes

riemannulus closed this Aug 12, 2020

riemannulus reopened this Aug 12, 2020

riemannulus self-requested a review August 12, 2020 07:37

riemannulus previously approved these changes Aug 12, 2020

View reviewed changes

limebell previously approved these changes Aug 12, 2020

View reviewed changes

Swarm<T>.GetTrustedStateCompleterAsync()

b9f0226

dahlia and others added 6 commits August 12, 2020 18:38

Extract out message handlers to a partial class

57d48e9

Print logs during BlockChainTest

e71ba8e

Prevent heterogenous chains being swapped

772558d

Trust other peers' states on reorged

b62447d

Add stateCompleters option to BlockChain<T>.Append()

1783127

Rewords

f0e715a

Co-authored-by: Seunghun Lee <waydi1@gmail.com>

dahlia dismissed stale reviews from limebell and riemannulus via f0e715a August 12, 2020 09:39

dahlia force-pushed the trusted-state-completer branch from 094e483 to f0e715a Compare August 12, 2020 09:39

Make SwarmTest to dispose all Swarm<T>s it makes

a2814ef

planetarium#946 (comment) planetarium#946 (comment)

dahlia requested review from longfin, earlbread, limebell and riemannulus August 12, 2020 12:01

dahlia added a commit to dahlia/libplanet that referenced this pull request Aug 12, 2020

Add write mode to state completers

733a4f4

planetarium#946 (comment)

earlbread approved these changes Aug 13, 2020

View reviewed changes

longfin approved these changes Aug 13, 2020

View reviewed changes

longfin merged commit 30bd5d7 into planetarium:main Aug 13, 2020

dahlia added a commit to dahlia/libplanet that referenced this pull request Aug 13, 2020

Add write mode to state completers

beeb045

planetarium#946 (comment)

dahlia added a commit to dahlia/libplanet that referenced this pull request Aug 13, 2020

Add write mode to state completers

aa0e865

planetarium#946 (comment) [changelog skip]

dahlia mentioned this pull request Aug 13, 2020

WIP: Add “write mode” to state completers #955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trusted state completer #946

Trusted state completer #946

dahlia commented Aug 6, 2020 •

edited

Loading

codecov bot commented Aug 6, 2020 •

edited

Loading

dahlia commented Aug 12, 2020

longfin Aug 12, 2020

dahlia Aug 12, 2020 •

edited

Loading

longfin Aug 12, 2020

dahlia Aug 12, 2020

longfin Aug 12, 2020 •

edited

Loading

dahlia Aug 13, 2020

longfin Aug 12, 2020 •

edited

Loading

dahlia Aug 12, 2020

dahlia Aug 13, 2020

dahlia commented Aug 13, 2020

	_rwlock.EnterUpgradeableReadLock();
	try
	{
	stateReference = Store.LookupStateReference(Id, key, block);

	if (stateReference is null)
	{
	return null;
	}

	HashDigest<SHA256> hashValue = stateReference.Item1;
	IImmutableDictionary<string, IValue> blockStates = Store.GetBlockStates(hashValue);
	if (blockStates is null)
	{
	return rawStateCompleter(this, hashValue);
	}

	return blockStates.TryGetValue(key, out IValue state) ? state : null;
	}
	finally
	{
	_rwlock.ExitUpgradeableReadLock();
	}

	_rwlock.EnterWriteLock();

	try
	{
	SetStates(block, evaluations, buildStateReferences: false);
	}
	finally
	{
	_rwlock.ExitWriteLock();
	}

Trusted state completer #946

Trusted state completer #946

Conversation

dahlia commented Aug 6, 2020 • edited Loading

codecov bot commented Aug 6, 2020 • edited Loading

Codecov Report

dahlia commented Aug 12, 2020

longfin Aug 12, 2020

Choose a reason for hiding this comment

dahlia Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

longfin Aug 12, 2020

Choose a reason for hiding this comment

dahlia Aug 12, 2020

Choose a reason for hiding this comment

longfin Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

dahlia Aug 13, 2020

Choose a reason for hiding this comment

longfin Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

dahlia Aug 12, 2020

Choose a reason for hiding this comment

dahlia Aug 13, 2020

Choose a reason for hiding this comment

dahlia commented Aug 13, 2020

dahlia commented Aug 6, 2020 •

edited

Loading

codecov bot commented Aug 6, 2020 •

edited

Loading

dahlia Aug 12, 2020 •

edited

Loading

longfin Aug 12, 2020 •

edited

Loading

longfin Aug 12, 2020 •

edited

Loading