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

Implemented support for note trees in Miden node #295

Merged
merged 6 commits into from
Apr 5, 2024
Merged

Implemented support for note trees in Miden node #295

merged 6 commits into from
Apr 5, 2024

Conversation

polydez
Copy link
Contributor

@polydez polydez commented Apr 2, 2024 •
edited
Loading

Updated code to use BlockTreeNotes and BatchTreeNotes, introduced batch_index in order to reduce bugs with calculations.

@polydez polydez changed the base branch from main to next April 2, 2024 15:23
@polydez polydez marked this pull request as ready for review April 2, 2024 15:25
@igamigo igamigo linked an issue Apr 3, 2024 that may be closed by this pull request
Add a wrapper type for the note position #276
Closed
5 tasks
igamigo
igamigo approved these changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

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

I did not check out the changes in miden-base in depth. I'm a bit confused by the introduction of the batch index and what advantages it brings but the overall code LGTM.

bobbinth
bobbinth approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few small comments inline. After these are addressed, we can merge.

store/src/db/migrations.rs Outdated
Comment on lines 21 to 22
batch_index INTEGER NOT NULL,
note_index INTEGER NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add comments here to clarify what batch_index and note_index represent.

store/src/db/migrations.rs Show resolved Hide resolved
faucet/Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
Comment on lines 27 to 31
miden-lib = { git = "https://github.com/0xPolygonMiden/miden-base.git", branch = "polydez-notes-tree" }
miden-objects = { git = "https://github.com/0xPolygonMiden/miden-base.git", branch = "polydez-notes-tree" }
miden-processor = { version = "0.8" }
miden-stdlib = { version = "0.8", default-features = false }
miden-tx = { version = "0.1" }
miden-tx = { git = "https://github.com/0xPolygonMiden/miden-base.git", branch = "polydez-notes-tree" }
Copy link
Contributor

Choose a reason for hiding this comment

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

These can probably be changed to the next branch now.

@polydez
Copy link
Contributor Author

polydez commented Apr 4, 2024

@igamigo

I did not check out the changes in miden-base in depth. I'm a bit confused by the introduction of the batch index and what advantages it brings but the overall code LGTM.

We need batch index for block note tree in order to mitigate bugs with note index calculations we faced. Our previous approach was to write note id and metadata hash into two adjacent leaves of SimpleSmt, which indexes were calculated as

MAX_NUMBER_OF_BATCHES * batch_index + note_index_in_batch * 2

and +1 for metadata hash. This code was duplicated several times and each implementation was different, some of them were buggy. This PR should force to provide batch index and node index in batch as separate arguments and tree implementation will calculate all the rest by itself in one single place.

@polydez polydez merged commit 1cbe5a8 into next Apr 5, 2024
5 checks passed
@polydez polydez deleted the polydez-note-trees branch April 5, 2024 06:34
@mFragaBA mFragaBA mentioned this pull request Apr 5, 2024
Merged
@igamigo igamigo mentioned this pull request Apr 6, 2024
Merged
bobbinth pushed a commit that referenced this pull request Apr 12, 2024
@polydez @bobbinth
Implemented support for note trees in Miden node (#295)
3b42e8d 
* feat: migration to the `next` versions of Miden dependencies

* fix: address review comments

* fix: update to the latest `next`

* feat: support of `BatchNoteTree` and `BlockNoteTree`

* feat: support of `BatchNoteTree` and `BlockNoteTree`, use `batch_index` instead of encoding it into the note index

* docs: add comments in SQL for batch and note indexes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igamigo igamigo igamigo approved these changes

@phklive phklive phklive left review comments

@bobbinth bobbinth bobbinth approved these changes

@hackaugusto hackaugusto Awaiting requested review from hackaugusto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a wrapper type for the note position
4 participants
@polydez @igamigo @phklive @bobbinth