Account compression: Init with root allowing trees with a canopy #6764

StanChe · 2024-05-27T10:03:38Z

Initializing a tree with a root

This PR modifies the account-compression program allowing the creation of the trees that were prepared offline. It allows creating trees with or without a canopy.
Verification of the validity, events, any additional checks and fields (URLs, hashes, etc.), and any additional permissions for the created tree are left to the discretion of the calling program.

The workflow

sequenceDiagram
autonumber
actor TC as Tree creator program
participant AC as account-compression
activate TC
TC ->> AC: prepare_batch_merkle_tree(tree_account, max_depth, max_buffer_size)
AC -> AC: initializes the header, but not the tree
loop For a tree with a canopy
TC ->> AC: append_canopy_nodes(tree_account, start_index, canopy_nodes)
end
TC ->> AC: init_prepared_tree_with_root(tree_account, root, rightmost_leaf, rightmost_index, proof)
AC -> AC: verifies the canopy is valid
AC -> AC: initializes the tree

The separation of a single init_with_root is based on the fact that some trees to be created might require knowing the pubkey of the tree, like with the bubblegum, where asset ids are derived from the tree pubkey and the index in that tree. Thus the user should first create the tree account and assign all the required permissions and then may take time to prepare the tree itself.

If the tree has a canopy - all canopy leaves should be initialized, otherwise the finalization will fail.

This change is inspired by #6441

ngundotra · 2024-05-31T19:32:17Z

Will look at this in mid June when back from OOO. Is there context behind why this needs to get merged into SPL Account Compression, rather than being done in a separate fork of the program?

StanChe · 2024-06-03T08:40:19Z

Integrating this feature into the standard library will benefit existing projects already utilizing account compression by enabling them to extend their contracts with rollups and batch creation of trees. This will reduce the number of transactions needed for tree creation, thereby alleviating network congestion. The batch creation method ensures that the tree is in the same state as it would be with individual appends, maintaining consistency. The prepared but not finalized tree will be unusable by other existing methods until it's finalized. The responsibility of validating the tree remains with the calling program, while this change guarantees that the tree initialized with a root has a valid canopy. This modification provides a tool for the batch creation of compressed accounts without imposing specific requirements on the caller and leaving the indexing up to their logic.

danenbm

I suspect we should fix the stack overflow thing. Looks like this program was last deployed about 4 months ago, maybe it wasn't an issue then, but I worry it could cause runtime issues now, even if the tests pass not sure if failures would be predictable.

Recently we fixed a similar stack overflow issue that started causing test failures in mpl-core-candy-machine by simply heap-allocating a large stack variable: metaplex-foundation/mpl-core-candy-machine@9575c8b

danenbm · 2024-06-04T19:56:00Z

account-compression/programs/account-compression/src/lib.rs

+        require_eq!(
+            *ctx.accounts.merkle_tree.owner,
+            crate::id(),
+            AccountCompressionError::IncorrectAccountOwner
+        );
+        let mut merkle_tree_bytes = ctx.accounts.merkle_tree.try_borrow_mut_data()?;
+
+        let (header_bytes, rest) =
+            merkle_tree_bytes.split_at_mut(CONCURRENT_MERKLE_TREE_HEADER_SIZE_V1);
+        // the header should already be initialized with prepare_tree
+        let header = ConcurrentMerkleTreeHeader::try_from_slice(header_bytes)?;
+        header.assert_valid_authority(&ctx.accounts.authority.key())?;
+        let merkle_tree_size = merkle_tree_get_size(&header)?;
+        let (tree_bytes, canopy_bytes) = rest.split_at_mut(merkle_tree_size);


Nit: this block seems like it could be refactored into a helper given its done the same in append_canopy_nodes and similar in prepare_tree (except for initializing header).

But this is a style thing, maybe its nicer to just have the flat structure as-is. Feel free to evaluate this comment and do whatever you think is best.

I've tried to support the existing code style as close to the original as possible. In a general case, most of the existing methods may be refactored to use that same helper method. In my opinion that may be done as a separate effort that doesn't modify the functionality and only refactores the code.

account-compression/programs/account-compression/src/lib.rs

danenbm · 2024-06-04T22:07:03Z

account-compression/sdk/tests/accountCompression.test.ts

Great tests!

StanChe · 2024-06-10T11:01:34Z

@danenbm thanks for the feedback. Decided to move the fix of the stack overflow into a separate PR #6827 in order to simplify this one and not to mix everything up.

ngundotra

This is really great PR overall. Great job extending existing design patterns (as bad as they are 😅).

High level requests:

There should be an easy way to discriminate normal trees from batch-init'd trees. When we talked previously, I suggested adding a new header version & a flag in that header.
What is the recourse for errors in batch-init'd trees? Which steps of the process are reversible or explicitly immutable? For the immutable steps, can we have some explicit documentation & also add helper methods to the SDK prevent immutable errors?

account-compression/programs/account-compression/src/lib.rs

Pull request has been modified.

ngundotra

What happens if I init a batch tree with prepare_tree and then attempt to append to it?

I think that another byte of padding is needed to store is_initialized for batch init'd trees. Then the tree can be checked quickly before executing replace or append. I'm open to more efficient ways of doing this - just wanted to offer my suggestion.

Again, great job with this. Almost there

ngundotra · 2024-07-25T16:48:24Z

account-compression/programs/account-compression/src/lib.rs

+    /// filled) -> `finalize_merkle_tree_with_root`. This instruction initializes the tree header
    /// while leaving the tree itself uninitialized. This allows distinguishing between an empty
    /// tree and a tree prepare to be initialized with a root.
    pub fn prepare_tree(


Can this method be renamed to init_batch_merkle_tree to match the other init method?

Also, what happens if this method is called on an already-existing tree?

Can this method be renamed to init_batch_merkle_tree to match the other init method?

If we take a look at the account structure it may be represented as | header | tree body | canopy |. The existing init_empty_merkle_tree does the full initialization - first the header is initialized, then the tree body, and finally the canopy size is checked. This makes the tree usable by the append methods.
For a batch initialized tree the actual initialization takes several steps. First the prepare_tree initializes just the header and checks the canopy size leaving the rest of the account zero'd. Then the canopy is set using append_canopy_nodes. And finally the finalize_merkle_tree_with_root initializes the tree body.
Until the finalize_merkle_tree_with_root is called the tree remains unusable by any other method - the init_empty_merkle_tree will fail because the header is initialized and any modification will fail because the tree itself is not initialized (the self.is_initialized() check will fail). Only the newly added append_canopy_nodes and finalize_merkle_tree_with_root as well as the modified close_empty_tree can operate over a tree in such a state.
That was also the reason the method naming is different to init_ as it leaves the tree in a not fully initialized state, but with only the initialized header (thus prepared).
I'm considering to rename the finalize_merkle_tree_with_root into init_prepared_tree_with_root and prepare_tree into prepare_batch_merkle_tree. This will keep the naming concise in terms the tree is fully initialized after the init_xxx methods. Will also update the comments accordingly. Wdyt?

Also, what happens if this method is called on an already-existing tree?

Adding a test to showcase this.

Gotcha, thanks for explaining. This makes sense. Can you add a simplified version of this comment as a docstring to the function tree_bytes_uninitialized ?

account-compression/programs/account-compression/src/lib.rs

Pull request has been modified.

StanChe · 2024-07-26T13:53:14Z

Also noticed the following stack overflow error occurring on build, fixed it as well

Error: Function _ZN23spl_account_compression23spl_account_compression22init_empty_merkle_tree17hbc30209ef0f8a270E Stack offset of 4928 exceeded max offset of 4096 by 832 bytes, please minimize large stack variables

ngundotra

Looks great. Almost ready to push to devnet for testing.

Two more comments:

Rebase on top of master
Address some small docs + typos

Once this is done, I'll add a PR that updates versions for & publishes the crates & js sdk.

ngundotra · 2024-07-29T14:48:39Z

account-compression/programs/account-compression/src/concurrent_tree_wrapper.rs

+    merkle_tree_apply_fn_mut!(header, tree_id, tree_bytes, initialize,)
+}
+
+pub fn tree_bytes_unititialized(tree_bytes: &[u8]) -> bool {


nit: typo in uninitialized

ngundotra · 2024-07-29T14:57:23Z

account-compression/programs/account-compression/src/lib.rs

+    /// filled) -> `finalize_merkle_tree_with_root`. This instruction initializes the tree header
    /// while leaving the tree itself uninitialized. This allows distinguishing between an empty
    /// tree and a tree prepare to be initialized with a root.
    pub fn prepare_tree(


Gotcha, thanks for explaining. This makes sense. Can you add a simplified version of this comment as a docstring to the function tree_bytes_uninitialized ?

account-compression/programs/account-compression/src/lib.rs

Pull request has been modified.

ngundotra · 2024-07-30T21:53:51Z

account-compression/programs/account-compression/src/canopy.rs

+    if canopy[node_idx - 2] != EMPTY {
+        return canopy[node_idx - 2];
+    }


If it wasn't for your extensive tests, I definitely would have thought this would be a footgun.

Converting node indexing to canopy indexing almost always throws me off

…tree_with_root as the merkle tree account will not be zeroed at that point

…better reflect its usage

… + minor fmt

…or emptiness

…d flag + comments fixed

…ounts + updated comments on append_canopy_nodes to reflect the possibility to replace those

…initialization call in a wrapper as it started reporting a stack overflow

…import

bumped the node version to the latest used in some other flows trying to narrow down the build issue

StanChe · 2024-09-11T21:55:36Z

FYI, had to bump up node to 20.5 for 2 js workflows that were failing on node 16. Looks like eslint became incompatible somehow. Used 20.5 as the latest used in some other flows, 18 should work as well.

mergify bot added the community Community contribution label May 27, 2024

StanChe marked this pull request as draft May 27, 2024 10:12

StanChe changed the title ~~Feature/init with root~~ Init with root allowing trees with canopy May 27, 2024

StanChe changed the title ~~Init with root allowing trees with canopy~~ Init with root allowing trees with a canopy May 27, 2024

StanChe force-pushed the feature/init_with_root branch from b208909 to 4756083 Compare May 27, 2024 10:16

StanChe marked this pull request as ready for review May 27, 2024 10:52

StanChe changed the title ~~Init with root allowing trees with a canopy~~ Account compression: Init with root allowing trees with a canopy May 27, 2024

joncinque requested a review from ngundotra May 28, 2024 22:12

danenbm reviewed Jun 4, 2024

View reviewed changes

StanChe force-pushed the feature/init_with_root branch from 612330e to 2191e25 Compare June 25, 2024 11:42

github-actions bot added the stale [bot only] Added to stale content; will be closed soon label Jul 12, 2024

ngundotra previously requested changes Jul 18, 2024

View reviewed changes

account-compression/programs/account-compression/src/lib.rs Outdated Show resolved Hide resolved

account-compression/programs/account-compression/src/lib.rs Show resolved Hide resolved

github-actions bot removed the stale [bot only] Added to stale content; will be closed soon label Jul 19, 2024

ngundotra previously requested changes Jul 25, 2024

View reviewed changes

ngundotra mentioned this pull request Jul 25, 2024

fix empty_node_cache use for cache #6578

Closed

ngundotra previously requested changes Jul 29, 2024

View reviewed changes

ngundotra reviewed Jul 30, 2024

View reviewed changes

github-actions bot added stale [bot only] Added to stale content; will be closed soon and removed stale [bot only] Added to stale content; will be closed soon labels Aug 19, 2024

github-actions bot added the stale [bot only] Added to stale content; will be closed soon label Sep 9, 2024

StanChe mentioned this pull request Sep 10, 2024

Batch operations support metaplex-foundation/blockbuster#48

Draft

StanChe force-pushed the feature/init_with_root branch from f343436 to da154fc Compare September 10, 2024 18:26

github-actions bot removed the stale [bot only] Added to stale content; will be closed soon label Sep 11, 2024

StanChe added 22 commits September 11, 2024 15:28

init_merkle_tree_with_root and refactored prepare_tree

1d94f80

rename init_merkle_tree_with_root into finalize_merkle_tree_with_root

4274c9b

using the Modify context for append_canopy_nodes and finalize_merkle_…

a4f8fcb

…tree_with_root as the merkle tree account will not be zeroed at that point

tests for preparing and finalizing an offline built tree

7193f09

cleanup and formatting for rust code

f2d2a46

regenerated sdk

a6e3c67

fmt/prettier on the new changes in js/ts

f8263c1

cleanup

eb80c93

cargo clippy over new code

c547cff

nit: updating a comment for Modify context on account compression to …

41e5017

…better reflect its usage

using the stackoverflow-safe version of the initialize_with_root call…

1a759e1

… + minor fmt

fixed an issue with addressing a canopy directly, without the check f…

6e7a347

…or emptiness

alligned the comment style for a constant with other comments

5dc6dd7

extended the concurrent merkle tree header with a is_batch_initialize…

d49dc5b

…d flag + comments fixed

updated close account to allow closing prepared and not finalized acc…

c94a3fa

…ounts + updated comments on append_canopy_nodes to reflect the possibility to replace those

adding more tests

48a7b96

refactored tree unitialized check on bytes directly, also moved tree …

1ddb5ce

…initialization call in a wrapper as it started reporting a stack overflow

typo fix + a docstring for tree_bytes_uninitialized + removed unused …

a936383

…import

naming changes

26fb7c8

lint the ts

29ec013

minor naming adjustment in comments

13ba21d

fix formatting, update lock file and generated idl versions

7cda6c4

StanChe force-pushed the feature/init_with_root branch from f343436 to 7cda6c4 Compare September 11, 2024 14:31

using node 20.5 for 2 workflows that are failing

6a3c9cb

bumped the node version to the latest used in some other flows trying to narrow down the build issue

ngundotra and others added 2 commits September 13, 2024 14:25

Merge branch 'master' into feature/init_with_root

7fab6b9

update versions for release

4d2153f

ngundotra merged commit b8615ab into solana-labs:master Sep 13, 2024
39 checks passed

ngundotra mentioned this pull request Sep 13, 2024

Account Compression v0.4.0 js release #7277

Merged

n00m4d mentioned this pull request Oct 16, 2024

Batch mint metaplex-foundation/blockbuster#50

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account compression: Init with root allowing trees with a canopy #6764

Account compression: Init with root allowing trees with a canopy #6764

StanChe commented May 27, 2024 •

edited

Loading

ngundotra commented May 31, 2024

StanChe commented Jun 3, 2024

danenbm left a comment

danenbm Jun 4, 2024 •

edited

Loading

StanChe Jun 10, 2024

danenbm Jun 4, 2024

StanChe commented Jun 10, 2024 •

edited

Loading

ngundotra left a comment

ngundotra left a comment

ngundotra Jul 25, 2024

ngundotra Jul 25, 2024

StanChe Jul 26, 2024

ngundotra Jul 29, 2024

StanChe commented Jul 26, 2024 •

edited

Loading

ngundotra left a comment

ngundotra Jul 29, 2024

ngundotra Jul 29, 2024

ngundotra Jul 30, 2024

StanChe commented Sep 11, 2024

Account compression: Init with root allowing trees with a canopy #6764

Account compression: Init with root allowing trees with a canopy #6764

Conversation

StanChe commented May 27, 2024 • edited Loading

Initializing a tree with a root

The workflow

ngundotra commented May 31, 2024

StanChe commented Jun 3, 2024

danenbm left a comment

Choose a reason for hiding this comment

danenbm Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanChe commented Jun 10, 2024 • edited Loading

ngundotra left a comment

Choose a reason for hiding this comment

ngundotra left a 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

StanChe commented Jul 26, 2024 • edited Loading

ngundotra left a 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

StanChe commented Sep 11, 2024

StanChe commented May 27, 2024 •

edited

Loading

danenbm Jun 4, 2024 •

edited

Loading

StanChe commented Jun 10, 2024 •

edited

Loading

StanChe commented Jul 26, 2024 •

edited

Loading