Find blocks in store instead of in chain

[area363]

This PR changes the BlockCompletion predicate to use the ContainsBlock method of IStore instead of BlockChain. This way, Swarm won't have to download blocks again if preloading is aborted during the Block Execution stage.

[dahlia]

Could you add a minimum test case to reproduce the problem this PR tries to address? Would it be difficult to reproduce in deterministic manner?

[longfin]

it would be better if we have changelog about this patch. (as behaviour change?)

[stale]

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]

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

[codecov]

Codecov Report

Merging #1324 (d1e40d3) into 0.12-maintenance (40e7735) will decrease coverage by 0.24%.
The diff coverage is 100.00%.

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

@@                 Coverage Diff                  @@
##           0.12-maintenance    #1324      +/-   ##
====================================================
- Coverage             77.34%   77.09%   -0.25%     
====================================================
  Files                   256      254       -2     
  Lines                 17229    17145      -84     
====================================================
- Hits                  13325    13218     -107     
- Misses                 3353     3371      +18     
- Partials                551      556       +5     

Impacted Files	Coverage Δ
Libplanet/Net/Swarm.cs	84.82% <100.00%> (-0.05%)	⬇️
Libplanet/Store/StoreExtensions.cs	64.70% <0.00%> (-11.16%)	⬇️
Libplanet/Net/SwarmOptions.cs	92.30% <0.00%> (-7.70%)	⬇️
Libplanet/ByteUtil.cs	88.23% <0.00%> (-5.42%)	⬇️
Libplanet/Net/Protocols/RoutingTable.cs	87.89% <0.00%> (-4.86%)	⬇️
Libplanet/Blocks/Block.cs	86.55% <0.00%> (-1.64%)	⬇️
Libplanet/Blockchain/BlockChain.cs	90.51% <0.00%> (-1.16%)	⬇️
Libplanet/Store/BlockSet.cs	84.48% <0.00%> (-0.52%)	⬇️
Libplanet.RocksDBStore/MonoRocksDBStore.cs	90.63% <0.00%> (-0.41%)	⬇️
Libplanet/Blockchain/Policies/BlockPolicy.cs	91.76% <0.00%> (-0.38%)	⬇️
... and 19 more

[limebell]

PTAL @libplanet

[kfangw]

LGTM.
I am not sure, but if there is performance issue(db read is expensive than memory read) then, how about the following?

• look up the blockchain first. If not exists, look up the store

[limebell]

LGTM.
I am not sure, but if there is performance issue(db read is expensive than memory read) then, how about the following?

• look up the blockchain first. If not exists, look up the store

Looking up in the blockchain looks up storage too.

[kfangw]

LGTM.
I am not sure, but if there is performance issue(db read is expensive than memory read) then, how about the following?

• look up the blockchain first. If not exists, look up the store

Looking up in the blockchain looks up storage too.

• BlockChain.ContainsBlock return true if blockchain and store both contains the block with other conditions
• Store.ContainsBlock return true if store contains the block

IMO, what I mentioned(in blockchain but not in store) is not covered by BlockChain.ContainsBlock

Btw, BlockChain.ContainsBlock already searching db too, hence there is no further problem with performance by this change.

[greymistcube]

I thought the main issue was unnecessarily redownloading a block where given block is not in blockchain but in store? Improving storage access performance is a separate issue.

[longfin]

/rebase

[dahlia]

/rebase

[longfin]

/rebase

[dahlia]

Retargeted to the 0.12-maintenance branch and rebased on that.