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

Write the Genesis block in the BlockStorage #306

Merged
merged 4 commits into from Sep 9, 2019

Conversation

AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Fixes #286

This or #194 will need a rebase depending on which gets pulled first. So I'll mark this a blocked for now.

@AndrejMitrovic AndrejMitrovic added the status-blocked label Sep 6, 2019
@AndrejMitrovic AndrejMitrovic added this to the 1. Full Node milestone Sep 6, 2019
@AndrejMitrovic AndrejMitrovic added this to In progress (Max 4) in Kanban period (August-September 2019) via automation Sep 6, 2019
@AndrejMitrovic AndrejMitrovic added the type-enhancement label Sep 6, 2019
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Fixed the integration tests.

@codecov
Copy link

@codecov codecov bot commented Sep 6, 2019

Codecov Report

Merging #306 into v0.x.x will decrease coverage by 0.43%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x     #306      +/-   ##
==========================================
- Coverage    81.7%   81.26%   -0.44%     
==========================================
  Files          49       49              
  Lines        2552     2583      +31     
==========================================
+ Hits         2085     2099      +14     
- Misses        467      484      +17
Flag Coverage Δ
#unittests 81.26% <86.36%> (-0.44%) ⬇️
Impacted Files Coverage Δ
tests/unit/BlockStorageChecksum.d 96.42% <ø> (-0.13%) ⬇️
tests/unit/BlockStorageMultiTx.d 100% <100%> (ø) ⬆️
source/agora/consensus/data/UTXOSet.d 85.71% <100%> (+0.93%) ⬆️
tests/unit/BlockStorage.d 97.82% <100%> (+0.04%) ⬆️
source/agora/node/BlockStorage.d 68.23% <55.55%> (-5.13%) ⬇️
source/agora/node/Ledger.d 93.78% <93.33%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13d58da...ddffbc8. Read the comment docs.

@@ -188,21 +194,32 @@ public class BlockStorage : IBlockStorage

if (!this.path.exists)
mkdirRecurse(this.path);
Copy link
Contributor

@Geod24 Geod24 Sep 6, 2019

Choose a reason for hiding this comment

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

I'd say this should be moved to load as well


***************************************************************************/

public bool isEmpty ()
public bool load ()
Copy link
Contributor

@Geod24 Geod24 Sep 6, 2019

Choose a reason for hiding this comment

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

It looks like the return value is only used in tests ? I don't think we need this information to be honest, it looks like a leaky abstraction...

Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic Sep 6, 2019

Choose a reason for hiding this comment

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

Like I mentioned before, I need to add the UTXO set from the genesis block in the UTXO PR.

I should only add the UTXOs if the genesis block was added for the very first time.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Ok put this on hold, I've got a simpler idea I'll work on later.

@AndrejMitrovic AndrejMitrovic removed the status-blocked label Sep 6, 2019
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

So, there is a better way to do what I wanted.

The ledger should check if the UTXO database is empty. If it's empty, it can walk through the ledger storage and add each block's UTXOs. Simple.

Unblocked.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Of course, it may be more efficient to get the mempool from the network in some cases, but for starters we can do it this way.

@Geod24 Geod24 moved this from In progress (Max 4) to Review/Testing (Max 2) in Kanban period (August-September 2019) Sep 6, 2019
If the UTXOSet is empty, it will need
to be rebuilt. Therefore we need to know
its length.
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Updated. There are no boolean return values anymore. It should be looked at commit by commit.

I still need to add one more test: if the UTXOSet was empty but the blockchain in the storage had multiple blocks, the UTXOSet should be properly re-built.

scope (failure) assert(0);

// add the new UTXOs
block.txs.each!(tx => this.utxo_set.updateUtxoCache(tx));
Copy link
Contributor

@Geod24 Geod24 Sep 8, 2019

Choose a reason for hiding this comment

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

UTXO, will need a rename

Geod24
Geod24 approved these changes Sep 8, 2019
Geod24
Geod24 approved these changes Sep 8, 2019
Copy link
Contributor

@Geod24 Geod24 left a comment

LGTM, just waiting for the last test you mentioned.

Consistency with acronym naming.
Rebuild the UTXOSet from the blockchain
which was loaded from disk.

This will also allow moving the adding
of the genesis block back to the BlockStorage.
Move the loading code into a separate load() method,
and get rid of the empty() method.

Loading should ideally never be done in the constructor.

Fixes bosagora#286
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 9, 2019

Updated with a test-case.

@Geod24 Geod24 merged commit da0c3e0 into bosagora:v0.x.x Sep 9, 2019
3 checks passed
Kanban period (August-September 2019) automation moved this from Review/Testing (Max 2) to Done Sep 9, 2019
@AndrejMitrovic AndrejMitrovic deleted the write-genesis branch Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants