Light History Store for full nodes #2370

viquezclaudio · 2024-04-15T14:59:42Z

Use the light history store for full node clients
Necessary changes to use the history interface
Several changes to the usage of the history store

...

This fixes #1704.

Pull request checklist

All tests pass. The project builds and runs.
I have resolved any merge conflicts.
I have resolved all clippy and rustfmt warnings.

sisou · 2024-04-16T03:45:05Z

Does this fix #1704?

viquezclaudio · 2024-04-16T05:32:50Z

Correct, this is the proper fix for 1704.
However this still needs more testing.

sisou · 2024-04-16T06:44:55Z

👍 I updated the PR description with that issue number.

paberr · 2024-04-17T16:41:25Z

blockchain/src/blockchain/history_sync.rs

+            if let Some(txns) = txns_per_block.get_mut(&bn) {
+                txns.push(txn.clone());
+            } else {
+                let new_txns = vec![txn.clone()];
+                txns_per_block.insert(bn, new_txns);
+            }


Suggested change

if let Some(txns) = txns_per_block.get_mut(&bn) {

txns.push(txn.clone());

} else {

let new_txns = vec![txn.clone()];

txns_per_block.insert(bn, new_txns);

}

txns_per_block.entry(bn).or_default().push(txn.clone());

paberr · 2024-04-17T16:44:03Z

blockchain/src/blockchain/history_sync.rs

+        for bn in txns_per_block.keys().sorted() {
+            let hist_txs = txns_per_block.get(bn).unwrap();


Using a BTreeMap, the keys would already be sorted.
Then, this could be simplified to for (bn, hist_txs) in txns_per_block.
Not sure whether it's worth it though.

Also, there's an inconsistency in naming: txs vs txns

Yes, I considered using a BTreeMap, but I wasn't sure if it was worth to add the dependency and use it vs just sorting the keys

if it was worth to add the dependency

It's in the standard library, so would be no additional dependency: std::collections::BTreeMap.

paberr · 2024-04-17T16:47:10Z

blockchain/src/history/history_store.rs

+
+        for hist_tx in ext_hash_vec {
+            // If the transaction is inside the validity window, return true.
+            if hist_tx.block_number >= validity_window_start {


Currently, we only check whether the transaction occurred after the validity window started.
However, it could potentially still fall outside the window if it occurred after it finished.
The second boundary check seems missing.

Here I'm not doing anything different than what we were doing before, i.e: I just moved this code: https://github.com/nimiq/core-rs-albatross/blob/albatross/blockchain/src/blockchain/wrappers.rs#L196
to the history store file.

Do you suggest that we used to have a bug?
What do you mean by "if it occurred after it finished." ?

paberr · 2024-04-17T16:50:41Z

blockchain/src/history/mmr_store.rs

+        // If size is 0 we need to copy the entries from the previous block number to the new one
+        if size == 0 {
+            init_mmr_from_block_number(hist_tree_table, tx, block_number);
+        }


I don't understand what this is about

For the light history store, what we are using are "peaks-only MMRs"
However, this means that if we have to revert blocks, we don't have enough information to just remove txns and go back to a previous state, as we do with the regular MMR.

So what we are doing is to store "peaks only mmrs" per block, i.e. essentially we have "snapshots" of how the peaks only MMR looked at each block.

However, since history nodes construct a big MMR with all the transactions of the given epoch, and this is the root that we need to verify against, then it means the starting point for block N is the peaks only mmr of block N-1, which is what this line is doing:
when we start pushing transactions for a block that we didn't have before, we first need to start with the peaks only mmr of the previous block and continue from there.

Let me know if this clarifies just question

paberr · 2024-04-17T16:56:32Z

blockchain/src/history/mmr_store.rs

+        };
+
+        // Deconstruct the key into a block number and a node index.
+        let (new_block_number, _) = key_to_index(next_key).unwrap();


key_to_index returns an epoch_number instead of a block_number (according to the variable names in the function; so this needs updating)

I'll update the comment, because we are using the same functions that the regular history store uses (which constructs keys based on {epoch_number, index} , but for the peaks only mmr, we construct keys based on {block_number, index}

paberr · 2024-04-17T17:06:46Z

blockchain/src/history/mmr_store.rs

@@ -232,6 +241,72 @@ pub fn remove_block_from_store(
    }
 }

+/// Copies the mmr entries of the previous block number
+/// This is used to initialize the tree for the given block number, using the previous tree as a baseline
+pub fn init_mmr_from_block_number(


What is the advantage of copying the contents over accessing the previous ones when needed?
Less complexity when accessing?

See my comment above.

Use the light history store for full node clients Necessary changes to use the history interface Several changes to usage of the history store

Mantain the txns in the validity store in database tables.

The history store range function now uses a database transaction Added unittest for total_len_at_epoch in the light history store

Add an unittest to exercise the transaction_in_validity_window functionality in the light history store Fixed typo in range inclusion when pruning the validity store

viquezclaudio added the WIP This pull request is still work in progress label Apr 15, 2024

viquezclaudio force-pushed the viquezcl/history_store branch 2 times, most recently from 98709f7 to 05143af Compare April 15, 2024 15:04

viquezclaudio force-pushed the viquezcl/history_store branch 3 times, most recently from b27ad62 to 62ad289 Compare April 16, 2024 20:33

paberr reviewed Apr 17, 2024

View reviewed changes

viquezclaudio force-pushed the viquezcl/history_store branch 2 times, most recently from 8eda34a to 240da36 Compare April 18, 2024 16:53

viquezclaudio force-pushed the viquezcl/history_store branch 2 times, most recently from 3f82095 to 59f9c4b Compare May 13, 2024 20:46

viquezclaudio force-pushed the viquezcl/history_store branch from 780e3c4 to 6eadd57 Compare May 15, 2024 17:03

viquezclaudio removed the WIP This pull request is still work in progress label May 15, 2024

viquezclaudio added 5 commits May 15, 2024 14:17

Light History Store for full nodes

b64a2d0

Use the light history store for full node clients Necessary changes to use the history interface Several changes to usage of the history store

Use the database for the validity store

f33ac00

Mantain the txns in the validity store in database tables.

Use BTreeMap when pushing txns during validity sync

a70fd0a

Fix history store range function

0f41696

The history store range function now uses a database transaction Added unittest for total_len_at_epoch in the light history store

Test transaction_in_validity_window

f7eec07

Add an unittest to exercise the transaction_in_validity_window functionality in the light history store Fixed typo in range inclusion when pruning the validity store

viquezclaudio force-pushed the viquezcl/history_store branch from 1ffc549 to f7eec07 Compare May 15, 2024 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Light History Store for full nodes #2370

Light History Store for full nodes #2370

viquezclaudio commented Apr 15, 2024 •

edited by sisou

sisou commented Apr 16, 2024

viquezclaudio commented Apr 16, 2024

sisou commented Apr 16, 2024

paberr Apr 17, 2024

paberr Apr 17, 2024

viquezclaudio Apr 17, 2024

hrxi Apr 29, 2024

paberr Apr 17, 2024

viquezclaudio Apr 17, 2024

paberr Apr 17, 2024

viquezclaudio Apr 17, 2024

paberr Apr 17, 2024

viquezclaudio Apr 17, 2024

paberr Apr 17, 2024

viquezclaudio Apr 17, 2024

		for bn in txns_per_block.keys().sorted() {
		let hist_txs = txns_per_block.get(bn).unwrap();

Light History Store for full nodes #2370

Are you sure you want to change the base?

Light History Store for full nodes #2370

Conversation

viquezclaudio commented Apr 15, 2024 • edited by sisou

This fixes #1704.

Pull request checklist

sisou commented Apr 16, 2024

viquezclaudio commented Apr 16, 2024

sisou commented Apr 16, 2024

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

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

viquezclaudio commented Apr 15, 2024 •

edited by sisou