fix: accumulated block data bitmap now contains current stxo indexes #3109

sdbondi · 2021-07-20T06:17:13Z

Description

Greatly reduces the amount of data stored per block by only storing the
spent indexes at a specific height instead of the entire spent bitmap.

The block chain database now stores a single full deleted bitmap at the
tip.

Breaking changes:

Blockchain Database (database must be reynced)
Blockchain Consensus (hard/soft fork)
P2P Network (some protocols are no longer compatible)
GRPC (GRPC clients need to be updated)

Motivation and Context

Bytes of storage per block reduced. At height 8033, the savings are
130mb on the new database. As the STXO set increases, the size of
the deleted bitmap typically increases (depends on compressibility of the data
in the bitmap), previously resulting in many megabytes of data per block.
This is no longer the case.

As a result of only having the tip bitmap, we can only calculate the output_mr for the next block
from the tip. In practice this is all that is needed, however some unit tests needed to be updated.

How Has This Been Tested?

Some unit tests updated.
Base node archival sync
Currently not able to test pruned sync since it is broken (fix: fix prune mode sync bug introduced in TariScript #3082)
Mined monero and sha3 blocks

Checklist:

I'm merging against the development branch.
I have squashed my commits into a single commit.

BREAKING CHANGE: Blockchain Database (database must be resynced)

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs

stringhandler · 2021-07-20T09:46:58Z

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs

@@ -264,6 +264,9 @@ impl LMDBDatabase {
                UpdateDeletedBlockAccumulatedDataWithDiff { header_hash, deleted } => {
                    self.update_deleted_block_accumulated_data_with_diff(&write_txn, header_hash, deleted)?;
                },
+                UpdateDeletedBitmap { deleted } => {


I don't like exposing this operation on its own. The deleted bitmap should only ever be updated as part of another operation, so that the database is alway consistent. Where is this operation called on it's own?

Agree with that, but trying to avoid refactoring existing code too much. It's used in pruned mode where there are a number of operations that must be atomic to not break the database and could potentially be brought together into one operation.

Let's add a comment there to discourage it's use

Greatly reduces the amount of data stored per block by only storing the spent indexes at a specific height instead of the entire spent bitmap. The block chain database now stores a single full deleted bitmap at the tip. BREAKING CHANGE: Blockchain database format is not backward-compatible

aviator-app · 2021-07-21T07:55:45Z

PR queued successfully. Your position in queue is: 2

aviator-app · 2021-07-21T08:16:49Z

PR is on top of the queue now

Prevent double spends from being added to the mempool. This bug was introduced by the blockchain deleted bitmap storage changes in tari-project#3109. A new block template contained already spent transactions which would then fail a double spend check in `calculate_mmr_roots`.

…3141)  ## Description  The main change here is to check the tip height after retrieving the block template but before MMR roots are calculated. When mining quickly in cucumber tests, this race condition occurs. Previously, this would be ok, the miner would simply mine 1 behind the current tip height. But because we can no longer calculate MMRs prior to the tip, this MMR call (`get_new_block`) now returns an error, which fails cucumber. `get_new_block` now returns a `FailedPrecondition` GRPC error status in the event that the block template submitted is not for the current chain tip. This allows clients to more easily identify this case, typically by asking for a new template. This refactor was performed because the protocol was gaining too much complexity to exist in a single function. Some extra changes to get all cucumbers to pass: - Replace `--daemon` flags with `--non-interactive` - Used more 'tolerant' step for `As a user I want to change base node for a wallet` scenario ## Motivation and Context   The `Verify all coinbases in hybrid mining are accounted for` cucumber test is flakey due to change in semantics for get_new_block since #3109. The possibility of this occurring with normal difficulties is usually tiny, but may be a symptom of a slow get_coinbase request. ## How Has This Been Tested?    Merge mined on local machine `Verify all coinbases in hybrid mining are accounted for` passes Suggest that we run tests with monero pool and nodejs pool ## Checklist:  * [x] I'm merging against the `development` branch. * [x] I have squashed my commits into a single commit.

sdbondi force-pushed the core-minimal-spent-bitmap branch 3 times, most recently from 93601b1 to 1a040a4 Compare July 20, 2021 06:32

sdbondi changed the title ~~Fix: Accumulated block data bitmap only contains current spent indexes~~ Fix: Accumulated block data bitmap now contains current spent indexes, db size reduction Jul 20, 2021

sdbondi changed the title ~~Fix: Accumulated block data bitmap now contains current spent indexes, db size reduction~~ fix: accumulated block data bitmap now contains current spent indexes, db size reduction Jul 20, 2021

sdbondi force-pushed the core-minimal-spent-bitmap branch from 1a040a4 to ff6ee27 Compare July 20, 2021 07:37

sdbondi changed the title ~~fix: accumulated block data bitmap now contains current spent indexes, db size reduction~~ fix: accumulated block data bitmap now contains current stxo indexes Jul 20, 2021

stringhandler reviewed Jul 20, 2021

View reviewed changes

sdbondi force-pushed the core-minimal-spent-bitmap branch 5 times, most recently from 6f5dd66 to ef69b32 Compare July 20, 2021 10:40

sdbondi force-pushed the core-minimal-spent-bitmap branch from ef69b32 to d844043 Compare July 20, 2021 10:42

aviator-app bot and others added 2 commits July 20, 2021 12:34

Merge branch 'development' into core-minimal-spent-bitmap

b3f16a3

Merge branch 'development' into core-minimal-spent-bitmap

74c96d8

stringhandler approved these changes Jul 21, 2021

View reviewed changes

stringhandler added the mq-approved label Jul 21, 2021

aviator-app bot merged commit 77b1789 into tari-project:development Jul 21, 2021

sdbondi deleted the core-minimal-spent-bitmap branch July 23, 2021 15:17

sdbondi mentioned this pull request Jul 26, 2021

fix: update transaction and block validator to use full deleted map #3137

Merged

2 tasks

sdbondi mentioned this pull request Jul 27, 2021

fix: improve reliability of get block template protocol in mm proxy #3141

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: accumulated block data bitmap now contains current stxo indexes #3109

fix: accumulated block data bitmap now contains current stxo indexes #3109

sdbondi commented Jul 20, 2021 •

edited by stringhandler

Loading

stringhandler Jul 20, 2021

sdbondi Jul 20, 2021 •

edited

Loading

stringhandler Jul 21, 2021

aviator-app bot commented Jul 21, 2021

aviator-app bot commented Jul 21, 2021

fix: accumulated block data bitmap now contains current stxo indexes #3109

fix: accumulated block data bitmap now contains current stxo indexes #3109

Conversation

sdbondi commented Jul 20, 2021 • edited by stringhandler Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist:

stringhandler Jul 20, 2021

Choose a reason for hiding this comment

sdbondi Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

stringhandler Jul 21, 2021

Choose a reason for hiding this comment

aviator-app bot commented Jul 21, 2021

aviator-app bot commented Jul 21, 2021

sdbondi commented Jul 20, 2021 •

edited by stringhandler

Loading

sdbondi Jul 20, 2021 •

edited

Loading