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.

Sign up for GitHub

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

Jump to bottom

fix: Empty blocks can now be unwound #11920

Merged
merged 6 commits into from
Feb 12, 2025
Merged

fix: Empty blocks can now be unwound #11920

merged 6 commits into from
Feb 12, 2025

Conversation

PhilWindle
Copy link
Collaborator

@PhilWindle PhilWindle commented Feb 11, 2025 •
edited
Loading

This PR has the following changes:

  1. Fixes an issue where empty blocks added from the genesis state could not be unwound.
  2. Refactors the retrieval of meta data so a transaction is not required if just reading uncommitted data.
  3. We now provide a specific method for committing the genesis state.

@PhilWindle PhilWindle added e2e-all CI: Deprecated, use ci-full. Enable all master checks. network-all CI: Deprecated, use ci-full. Runs all CI master checks. labels Feb 11, 2025
PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...g/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp
@@ -629,8 +644,42 @@ fr ContentAddressedCachedTreeStore<LeafValueType>::get_current_root(ReadTransact
// It is assumed that when these operations are being executed that no other state accessing operations
// are in progress, hence no data synchronisation is used.

template <typename LeafValueType> void ContentAddressedCachedTreeStore<LeafValueType>::commit_genesis_state()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some refactoring here. I created a specific method for commiting the genesis state.

PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...g/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp
template <typename LeafValueType>
void ContentAddressedCachedTreeStore<LeafValueType>::commit(TreeMeta& finalMeta, TreeDBStats& dbStats, bool asBlock)
void ContentAddressedCachedTreeStore<LeafValueType>::commit_block(TreeMeta& finalMeta, TreeDBStats& dbStats)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got rid of the asBlock arg. This was always tru except wjhen committing genesis state, which is now a dedicated method.

PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...berg/cpp/src/barretenberg/crypto/merkle_tree/indexed_tree/content_addressed_indexed_tree.hpp
ReadTransactionPtr tx = store_->create_read_transaction();
store_->get_meta(meta, *tx, false);
}
store_->get_meta(meta);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You will see this in a few places. I got rid of the need to pass a transaction just to read uncommitted meta data. The transaction was unused.

PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...g/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp
// absence of a real tree elsewhere. So, if the tree is completely empty we do not store any node data, the
// only issue is this needs to be recognised when we unwind or remove historic blocks i.e. there will be no
// node date to remove for these blocks
if (dataPresent || uncommittedMeta.size > 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The actual fix is subtle. If the very first block is empty then there is no data in the tree and the root is the 'zero' root. In this situation we can't write nodes to the store as to do so is meaningless (should we write the entire tree?!). I did wonder if we should write the root node, but this causes lots of problems elsewhere as there is no tree beneath it.

So we continue to leave the node store unpopulated until such time as the tree becomes populated with at least one leaf/node.

PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...g/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp
forkConstantData_.name_,
" Error: ",
e.what()));
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This just encapsulates the write tx in a scope block.

PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...g/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp
e.what()));

{
WriteTransactionPtr writeTx = create_write_transaction();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same again, scope block.

PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...g/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp

// If the tree was empty at the block being removed then we should not attempt to remove
// any nodes. (there were no nodes at the point this block was comitted)
if (blockData.size > 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is the other part of the fix. We don't attempt to remove nodes if at this point the tree was still empty.

PhilWindle
PhilWindle commented Feb 11, 2025
View reviewed changes
...g/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp
try {
// If the tree was empty at the block being removed then we should not attempt to remove
// any nodes. (there were no nodes at the point this block was comitted)
if (blockData.size > 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And again, don't attempt to remove historical nodes if the tree was empty at the time.

@PhilWindle PhilWindle linked an issue Feb 12, 2025 that may be closed by this pull request
WorldState can not unwind empty blocks #11688
Closed
@PhilWindle PhilWindle merged commit fdc2042 into master Feb 12, 2025
85 checks passed
@PhilWindle PhilWindle deleted the pw/unwind-empty-blocks-2 branch February 12, 2025 15:26
@AztecBot AztecBot mentioned this pull request Feb 12, 2025
Merged
sklppy88 pushed a commit that referenced this pull request Feb 13, 2025
@AztecBot
chore(master): Release 0.76.3 (#11924)
47fe362 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.76.3</summary>

##
[0.76.3](aztec-package-v0.76.2...aztec-package-v0.76.3)
(2025-02-12)


### Features

* Add undici
([#11818](#11818))
([8503c7a](8503c7a))
* Initial multi-proof test
([#11779](#11779))
([f54db75](f54db75))
</details>

<details><summary>barretenberg.js: 0.76.3</summary>

##
[0.76.3](barretenberg.js-v0.76.2...barretenberg.js-v0.76.3)
(2025-02-12)


### Features

* **perf:** Speed up construction of bbjs Frs & cache zero hashes in
ephemeral trees (redo)
([#11894](#11894))
([e093acf](e093acf))


### Bug Fixes

* Memory fragmentation fixes to cut UltraHonk memory usage by 26%
([#11895](#11895))
([b4e2264](b4e2264))
</details>

<details><summary>aztec-packages: 0.76.3</summary>

##
[0.76.3](aztec-packages-v0.76.2...aztec-packages-v0.76.3)
(2025-02-12)


### Features

* Add undici
([#11818](#11818))
([8503c7a](8503c7a))
* **avm:** Sequential lookup resolution
([#11769](#11769))
([3980f6c](3980f6c))
* Enable ws for reth on devnet
([#11922](#11922))
([7124664](7124664)),
closes
[#11921](#11921)
* Initial multi-proof test
([#11779](#11779))
([f54db75](f54db75))
* Native world state now supports checkpointing
([#11739](#11739))
([6464059](6464059))
* **perf:** Speed up construction of bbjs Frs & cache zero hashes in
ephemeral trees (redo)
([#11894](#11894))
([e093acf](e093acf))


### Bug Fixes

* Correctly configure batch queue
([#11934](#11934))
([4908df8](4908df8))
* De-dup pubkey conversion of cli-wallet param
([#11948](#11948))
([5529871](5529871))
* **docs:** Update the token bridge tutorial
([#11578](#11578))
([aaf42a7](aaf42a7))
* Don't restart kind control plane automatically
([#11923](#11923))
([c23c0f9](c23c0f9))
* Empty blocks can now be unwound
([#11920](#11920))
([fdc2042](fdc2042))
* Gcloud logs
([#11944](#11944))
([c53b1c5](c53b1c5)),
closes
[#11887](#11887)
* Lmdb cmake race condition
([#11959](#11959))
([031200d](031200d))
* Memory fragmentation fixes to cut UltraHonk memory usage by 26%
([#11895](#11895))
([b4e2264](b4e2264))
* Npm packages noir resolution
([#11946](#11946))
([d3e3f20](d3e3f20))
* Set resource limits on Loki pods
([#11940](#11940))
([6999982](6999982))


### Miscellaneous

* Only take FF (and not Flavor) in compute_logderivative_inverse
([#11938](#11938))
([bbbded3](bbbded3))
* Op queue cleanup
([#11925](#11925))
([082ed66](082ed66))
* Renaming `pxe_db.nr` as `capsules.nr`
([#11905](#11905))
([14d873c](14d873c))
* Replace relative paths to noir-protocol-circuits
([9ce401a](9ce401a))
* Use realistic size Client IVC proofs during simulations
([#11692](#11692))
([90b9fbf](90b9fbf))
* Use RelationChecker in relation correctness tests and add Translator
interleaving test
([#11878](#11878))
([ed215e8](ed215e8))
</details>

<details><summary>barretenberg: 0.76.3</summary>

##
[0.76.3](barretenberg-v0.76.2...barretenberg-v0.76.3)
(2025-02-12)


### Features

* **avm:** Sequential lookup resolution
([#11769](#11769))
([3980f6c](3980f6c))
* Native world state now supports checkpointing
([#11739](#11739))
([6464059](6464059))


### Bug Fixes

* Empty blocks can now be unwound
([#11920](#11920))
([fdc2042](fdc2042))
* Lmdb cmake race condition
([#11959](#11959))
([031200d](031200d))
* Memory fragmentation fixes to cut UltraHonk memory usage by 26%
([#11895](#11895))
([b4e2264](b4e2264))


### Miscellaneous

* Only take FF (and not Flavor) in compute_logderivative_inverse
([#11938](#11938))
([bbbded3](bbbded3))
* Op queue cleanup
([#11925](#11925))
([082ed66](082ed66))
* Use RelationChecker in relation correctness tests and add Translator
interleaving test
([#11878](#11878))
([ed215e8](ed215e8))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Feb 14, 2025
@AztecBot
chore(master): Release 0.76.3 (#11924)
ad2e48f 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.76.3</summary>

##
[0.76.3](AztecProtocol/aztec-packages@aztec-package-v0.76.2...aztec-package-v0.76.3)
(2025-02-12)


### Features

* Add undici
([#11818](AztecProtocol/aztec-packages#11818))
([8503c7a](AztecProtocol/aztec-packages@8503c7a))
* Initial multi-proof test
([#11779](AztecProtocol/aztec-packages#11779))
([f54db75](AztecProtocol/aztec-packages@f54db75))
</details>

<details><summary>barretenberg.js: 0.76.3</summary>

##
[0.76.3](AztecProtocol/aztec-packages@barretenberg.js-v0.76.2...barretenberg.js-v0.76.3)
(2025-02-12)


### Features

* **perf:** Speed up construction of bbjs Frs & cache zero hashes in
ephemeral trees (redo)
([#11894](AztecProtocol/aztec-packages#11894))
([e093acf](AztecProtocol/aztec-packages@e093acf))


### Bug Fixes

* Memory fragmentation fixes to cut UltraHonk memory usage by 26%
([#11895](AztecProtocol/aztec-packages#11895))
([b4e2264](AztecProtocol/aztec-packages@b4e2264))
</details>

<details><summary>aztec-packages: 0.76.3</summary>

##
[0.76.3](AztecProtocol/aztec-packages@aztec-packages-v0.76.2...aztec-packages-v0.76.3)
(2025-02-12)


### Features

* Add undici
([#11818](AztecProtocol/aztec-packages#11818))
([8503c7a](AztecProtocol/aztec-packages@8503c7a))
* **avm:** Sequential lookup resolution
([#11769](AztecProtocol/aztec-packages#11769))
([3980f6c](AztecProtocol/aztec-packages@3980f6c))
* Enable ws for reth on devnet
([#11922](AztecProtocol/aztec-packages#11922))
([7124664](AztecProtocol/aztec-packages@7124664)),
closes
[#11921](AztecProtocol/aztec-packages#11921)
* Initial multi-proof test
([#11779](AztecProtocol/aztec-packages#11779))
([f54db75](AztecProtocol/aztec-packages@f54db75))
* Native world state now supports checkpointing
([#11739](AztecProtocol/aztec-packages#11739))
([6464059](AztecProtocol/aztec-packages@6464059))
* **perf:** Speed up construction of bbjs Frs & cache zero hashes in
ephemeral trees (redo)
([#11894](AztecProtocol/aztec-packages#11894))
([e093acf](AztecProtocol/aztec-packages@e093acf))


### Bug Fixes

* Correctly configure batch queue
([#11934](AztecProtocol/aztec-packages#11934))
([4908df8](AztecProtocol/aztec-packages@4908df8))
* De-dup pubkey conversion of cli-wallet param
([#11948](AztecProtocol/aztec-packages#11948))
([5529871](AztecProtocol/aztec-packages@5529871))
* **docs:** Update the token bridge tutorial
([#11578](AztecProtocol/aztec-packages#11578))
([aaf42a7](AztecProtocol/aztec-packages@aaf42a7))
* Don't restart kind control plane automatically
([#11923](AztecProtocol/aztec-packages#11923))
([c23c0f9](AztecProtocol/aztec-packages@c23c0f9))
* Empty blocks can now be unwound
([#11920](AztecProtocol/aztec-packages#11920))
([fdc2042](AztecProtocol/aztec-packages@fdc2042))
* Gcloud logs
([#11944](AztecProtocol/aztec-packages#11944))
([c53b1c5](AztecProtocol/aztec-packages@c53b1c5)),
closes
[#11887](AztecProtocol/aztec-packages#11887)
* Lmdb cmake race condition
([#11959](AztecProtocol/aztec-packages#11959))
([031200d](AztecProtocol/aztec-packages@031200d))
* Memory fragmentation fixes to cut UltraHonk memory usage by 26%
([#11895](AztecProtocol/aztec-packages#11895))
([b4e2264](AztecProtocol/aztec-packages@b4e2264))
* Npm packages noir resolution
([#11946](AztecProtocol/aztec-packages#11946))
([d3e3f20](AztecProtocol/aztec-packages@d3e3f20))
* Set resource limits on Loki pods
([#11940](AztecProtocol/aztec-packages#11940))
([6999982](AztecProtocol/aztec-packages@6999982))


### Miscellaneous

* Only take FF (and not Flavor) in compute_logderivative_inverse
([#11938](AztecProtocol/aztec-packages#11938))
([bbbded3](AztecProtocol/aztec-packages@bbbded3))
* Op queue cleanup
([#11925](AztecProtocol/aztec-packages#11925))
([082ed66](AztecProtocol/aztec-packages@082ed66))
* Renaming `pxe_db.nr` as `capsules.nr`
([#11905](AztecProtocol/aztec-packages#11905))
([14d873c](AztecProtocol/aztec-packages@14d873c))
* Replace relative paths to noir-protocol-circuits
([9ce401a](AztecProtocol/aztec-packages@9ce401a))
* Use realistic size Client IVC proofs during simulations
([#11692](AztecProtocol/aztec-packages#11692))
([90b9fbf](AztecProtocol/aztec-packages@90b9fbf))
* Use RelationChecker in relation correctness tests and add Translator
interleaving test
([#11878](AztecProtocol/aztec-packages#11878))
([ed215e8](AztecProtocol/aztec-packages@ed215e8))
</details>

<details><summary>barretenberg: 0.76.3</summary>

##
[0.76.3](AztecProtocol/aztec-packages@barretenberg-v0.76.2...barretenberg-v0.76.3)
(2025-02-12)


### Features

* **avm:** Sequential lookup resolution
([#11769](AztecProtocol/aztec-packages#11769))
([3980f6c](AztecProtocol/aztec-packages@3980f6c))
* Native world state now supports checkpointing
([#11739](AztecProtocol/aztec-packages#11739))
([6464059](AztecProtocol/aztec-packages@6464059))


### Bug Fixes

* Empty blocks can now be unwound
([#11920](AztecProtocol/aztec-packages#11920))
([fdc2042](AztecProtocol/aztec-packages@fdc2042))
* Lmdb cmake race condition
([#11959](AztecProtocol/aztec-packages#11959))
([031200d](AztecProtocol/aztec-packages@031200d))
* Memory fragmentation fixes to cut UltraHonk memory usage by 26%
([#11895](AztecProtocol/aztec-packages#11895))
([b4e2264](AztecProtocol/aztec-packages@b4e2264))


### Miscellaneous

* Only take FF (and not Flavor) in compute_logderivative_inverse
([#11938](AztecProtocol/aztec-packages#11938))
([bbbded3](AztecProtocol/aztec-packages@bbbded3))
* Op queue cleanup
([#11925](AztecProtocol/aztec-packages#11925))
([082ed66](AztecProtocol/aztec-packages@082ed66))
* Use RelationChecker in relation correctness tests and add Translator
interleaving test
([#11878](AztecProtocol/aztec-packages#11878))
([ed215e8](AztecProtocol/aztec-packages@ed215e8))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexghr alexghr alexghr approved these changes

Assignees
No one assigned
Labels
e2e-all CI: Deprecated, use ci-full. Enable all master checks. network-all CI: Deprecated, use ci-full. Runs all CI master checks.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WorldState can not unwind empty blocks
2 participants
@PhilWindle @alexghr