[HackerOne-2239704] Introduce an override for atomic batch operations #2296

ljedrz · 2024-01-10T18:47:50Z

This PR introduces a database-level override flag that does not allow an atomic operation to be executed until it's flipped manually.

This is a relatively simple solution to make block insertion and finalization happen as a single atomic storage operation; the reason why they can't be carried out like that "normally" is that they are disjoint in the storage hierarchy.

This should fix the storage corruption that can currently happen if the node is shut down between those two operations.

Filing it as a draft, as I'd like to further improve the related docs, but it worked when I tested it locally.

This PR should address:

ljedrz · 2024-01-10T19:42:09Z

I'm also going to check the failing test cases; it's likely that some of them are just using the low-level operations individually.

Edit: it was actually just due to the in-memory storage making assertions that only apply to the persistent one; fixed.

ledger/store/src/helpers/rocksdb/internal/mod.rs

ljedrz · 2024-01-19T09:22:48Z

Rebased and addressed the review comment in a new commit.

ledger/store/src/block/mod.rs

ledger/store/src/helpers/rocksdb/internal/mod.rs

Signed-off-by: ljedrz <ljedrz@gmail.com>

ljedrz · 2024-01-20T10:22:12Z

Rebased and addressed the latest review comments in a new commit.

I also noticed that the unhappy path for finalization needed to be adjusted, which was done in the last commit.

synthesizer/src/vm/mod.rs

Signed-off-by: ljedrz <ljedrz@gmail.com>

synthesizer/src/vm/mod.rs

howardwu · 2024-02-18T20:58:12Z

synthesizer/src/vm/mod.rs

@@ -335,12 +335,32 @@ impl<N: Network, C: ConsensusStorage<N>> VM<N, C> {
        // Attention: The following order is crucial because if 'finalize' fails, we can rollback the block.
        // If one first calls 'finalize', then calls 'insert(block)' and it fails, there is no way to rollback 'finalize'.

+        // Enable the atomic batch override, so that both the insertion and finalization belong to a single batch.
+        #[cfg(feature = "rocks")]
+        assert!(self.block_store().flip_atomic_override()?);


Should this be debug_assert!?

the plain assert was intentional, as this ensures that it is initially disabled - if this was not the case, it would indicate a logic bug, and other atomic operations could be affected

ledger/store/src/helpers/rocksdb/internal/nested_map.rs

howardwu · 2024-02-18T22:17:44Z

synthesizer/src/vm/mod.rs

+                    self.finalize_store().atomic_abort();
+                    // Disable the atomic batch override.
+                    assert!(!self.block_store().flip_atomic_override()?);
+                }
                // Rollback the block.
                self.block_store().remove_last_n(1).map_err(|removal_error| {


This is still incorrect.

Calling remove_last_n(1) here will 1) correctly update the Merkle tree, however 2) it will also delete the penultimate block now from self.block_store().

This is erroneous because we are using this concept of a flip_atomic_override to remove the pending last block, so the call to remove_last_n(1) will now remove the penultimate block erroneously.

Indeed; there are 2 ways I can see of tackling this; we can either:

put the storage deletion within remove_last_n behind a feature flag (so that it doesn't happen with rocks); however, this would obscure the purpose of the method and affect its use outside of typical runtime (and probably break some tests)

break the remove_last_n method into 2 distinct operations (isolated into new methods) - the tree update and the storage update - and only call the tree update here; I'll see if I can propose something "clean" for this

I proposed the 2nd approach in ProvableHQ@1a27305. The scope of the write lock over the Merkle tree is different, but since the entirety of block insertion is behind the block_lock, this shouldn't affect anything (I'm assuming that only block insertion/rollback can alter it).

Signed-off-by: ljedrz <ljedrz@gmail.com>

ljedrz · 2024-02-21T11:58:24Z

~~Switching to draft so I can double-check the updated setup.~~

Done, and tested locally.

Signed-off-by: ljedrz <ljedrz@gmail.com>

raychu86

LGTM

ljedrz requested a review from howardwu January 10, 2024 18:47

ljedrz force-pushed the feat/atomic_batch_override branch 2 times, most recently from 6a17280 to 64d36d7 Compare January 11, 2024 12:02

ljedrz marked this pull request as ready for review January 11, 2024 13:23

ljedrz force-pushed the feat/atomic_batch_override branch from 64d36d7 to d4eb70b Compare January 11, 2024 13:25

raychu86 reviewed Jan 19, 2024

View reviewed changes

ledger/store/src/helpers/rocksdb/internal/mod.rs Show resolved Hide resolved

ljedrz force-pushed the feat/atomic_batch_override branch from d4eb70b to 5f50c0f Compare January 19, 2024 09:18

ljedrz mentioned this pull request Jan 19, 2024

[Bug] Failure to sync client node ProvableHQ/snarkOS#2996

Open

howardwu reviewed Jan 20, 2024

View reviewed changes

ledger/store/src/block/mod.rs Outdated Show resolved Hide resolved

howardwu reviewed Jan 20, 2024

View reviewed changes

ledger/store/src/block/mod.rs Outdated Show resolved Hide resolved

howardwu reviewed Jan 20, 2024

View reviewed changes

ledger/store/src/helpers/rocksdb/internal/mod.rs Show resolved Hide resolved

ljedrz added 4 commits January 20, 2024 11:19

feat: introduce an override for atomic batch operations

19a0bd0

Signed-off-by: ljedrz <ljedrz@gmail.com>

tweak: disallow atomic override flips in the middle of batches

adf3aad

Signed-off-by: ljedrz <ljedrz@gmail.com>

docs: improve atomic batch override notes

ff761ae

Signed-off-by: ljedrz <ljedrz@gmail.com>

fix: adjust the storage logic in case of finalization error

6bfa9a3

Signed-off-by: ljedrz <ljedrz@gmail.com>

ljedrz force-pushed the feat/atomic_batch_override branch from 5f50c0f to 6bfa9a3 Compare January 20, 2024 10:19

ljedrz mentioned this pull request Feb 9, 2024

[Bug] snarkOS sometimes would still corrupt the storage when syncing ProvableHQ/snarkOS#2974

Closed

raychu86 reviewed Feb 9, 2024

View reviewed changes

synthesizer/src/vm/mod.rs Outdated Show resolved Hide resolved

fix: call remove_last_n on finalization failure with persistent storage

2631882

Signed-off-by: ljedrz <ljedrz@gmail.com>

howardwu reviewed Feb 18, 2024

View reviewed changes

synthesizer/src/vm/mod.rs Outdated Show resolved Hide resolved

howardwu reviewed Feb 18, 2024

View reviewed changes

synthesizer/src/vm/mod.rs Outdated Show resolved Hide resolved

howardwu reviewed Feb 18, 2024

View reviewed changes

ledger/store/src/helpers/rocksdb/internal/nested_map.rs Outdated Show resolved Hide resolved

nit: rm deprecated comment; add readability scope

e2e207d

howardwu reviewed Feb 18, 2024

View reviewed changes

howardwu and others added 2 commits February 18, 2024 15:16

Merge branch 'testnet3' into feat/atomic_batch_override

ac3d8b8

refactor: isolate a method for the atomic override check

14a1136

Signed-off-by: ljedrz <ljedrz@gmail.com>

fix: don't remove the last block from rocksdb on finalization failure

1a27305

Signed-off-by: ljedrz <ljedrz@gmail.com>

ljedrz marked this pull request as draft February 21, 2024 11:58

refactor: split flip_atomic_override into (un)pause_atomic_writes

f656d9f

Signed-off-by: ljedrz <ljedrz@gmail.com>

ljedrz force-pushed the feat/atomic_batch_override branch from 6e5e390 to f656d9f Compare February 21, 2024 12:18

ljedrz marked this pull request as ready for review February 21, 2024 13:15

raychu86 approved these changes Feb 21, 2024

View reviewed changes

raychu86 changed the title ~~Introduce an override for atomic batch operations~~ [HackerOne-2239704] Introduce an override for atomic batch operations Feb 21, 2024

Adds safety on unpause_atomic_writes, reduces tree locking on removals

dd685ac

howardwu approved these changes Feb 21, 2024

View reviewed changes

howardwu merged commit d23daa8 into ProvableHQ:testnet3 Feb 21, 2024
70 of 78 checks passed

raychu86 mentioned this pull request Feb 21, 2024

Validator cannot recover from a crash and will be out of sync #2218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HackerOne-2239704] Introduce an override for atomic batch operations #2296

[HackerOne-2239704] Introduce an override for atomic batch operations #2296

ljedrz commented Jan 10, 2024 •

edited by raychu86

Loading

ljedrz commented Jan 10, 2024 •

edited

Loading

ljedrz commented Jan 19, 2024

ljedrz commented Jan 20, 2024

howardwu Feb 18, 2024

ljedrz Feb 19, 2024 •

edited

Loading

howardwu Feb 18, 2024 •

edited

Loading

ljedrz Feb 19, 2024

ljedrz Feb 19, 2024 •

edited

Loading

ljedrz commented Feb 21, 2024 •

edited

Loading

raychu86 left a comment

[HackerOne-2239704] Introduce an override for atomic batch operations #2296

[HackerOne-2239704] Introduce an override for atomic batch operations #2296

Conversation

ljedrz commented Jan 10, 2024 • edited by raychu86 Loading

ljedrz commented Jan 10, 2024 • edited Loading

ljedrz commented Jan 19, 2024

ljedrz commented Jan 20, 2024

howardwu Feb 18, 2024

Choose a reason for hiding this comment

ljedrz Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

howardwu Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

ljedrz Feb 19, 2024

Choose a reason for hiding this comment

ljedrz Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

ljedrz commented Feb 21, 2024 • edited Loading

raychu86 left a comment

Choose a reason for hiding this comment

ljedrz commented Jan 10, 2024 •

edited by raychu86

Loading

ljedrz commented Jan 10, 2024 •

edited

Loading

ljedrz Feb 19, 2024 •

edited

Loading

howardwu Feb 18, 2024 •

edited

Loading

ljedrz Feb 19, 2024 •

edited

Loading

ljedrz commented Feb 21, 2024 •

edited

Loading