Return the list of committed transactions in state sync endpoint #377

polydez · 2024-06-06T04:43:30Z

Resolves: #371
Related issue: 0xPolygonMiden/miden-base#722

mFragaBA

code looks good! I left some minor questions. I'll test this against the client to have another layer of validation

mFragaBA · 2024-06-06T21:09:46Z

crates/block-producer/src/block_builder/prover/mod.rs

+            .iter()
+            .flat_map(|(&account_id, update)| {
+                update.transactions.iter().flat_map(move |&tx| {
+                    let mut result = vec![Felt::ZERO, Felt::ZERO, Felt::ZERO, account_id.into()];


why do we need to add the account id to the elements to hash?

Please look into this issue for clarifications: 0xPolygonMiden/miden-base#722

mFragaBA · 2024-06-06T21:21:37Z

crates/store/src/db/sql.rs

+        ORDER BY
+            transaction_id ASC


does the order by transaction id add anything? was it supposed to be ordered by block number?

I just wanted to have reproducible results. If for client it's matter to have ordering by block number, I can change it.

I don't think any ordering would matter for the client, but it makes sense for reproducible results.

mFragaBA · 2024-06-06T21:26:17Z

crates/store/src/db/tests.rs

+#[test]
+fn test_sql_select_transactions() {
+    fn query_transactions(conn: &mut Connection) -> Vec<RpoDigest> {
+        sql::select_transactions_by_accounts_and_block_range(conn, 1, 2, &[1]).unwrap()
+    }
+
+    let mut conn = create_db();
+
+    let transactions = query_transactions(&mut conn);
+
+    assert!(transactions.is_empty(), "No elements must be initially in the DB");
+
+    let count = insert_transactions(&mut conn);
+
+    assert_eq!(count, 2, "Two elements must have been inserted");
+}


can we also query after inserting the transactions and make sure it's also 2?

Good catch, thank you! I moved query code to separated function in this matter, but forgot to query for a second time. :(

Cargo.toml

crates/store/src/state.rs

bobbinth

Looks good! Thank you! I left a couple of small comments inline.

Also, I think one thing that is missing is verifying that tx_hash is consistent with transaction info in the block during apply_block (we check that all other data in the block is consistent with the header, would be good to check that transaction data is consistent as well).

Lastly, what do you think of moving transaction IDs into AccountSummary (something along the lines what is suggested in #371 (comment)). I don't think we should do it in this PR, but curious how much work it would be.

crates/proto/proto/responses.proto

crates/block-producer/src/block_builder/prover/mod.rs

bobbinth · 2024-06-07T08:54:18Z

Oh - also, let's resolve merge conflicts and update the changelog.

# Conflicts: # Cargo.lock # Cargo.toml # crates/store/src/state.rs

polydez · 2024-06-07T12:19:29Z

@bobbinth

Lastly, what do you think of moving transaction IDs into AccountSummary (something along the lines what is suggested in #371 (comment)). I don't think we should do it in this PR, but curious how much work it would be.

I looked into the code and it seems to be pretty easy to implement this.

igamigo

LGTM, left a couple of comments (also, I wasn't sure if the AccountSummary updates are meant to be included in this PR or a follow-up).

igamigo · 2024-06-07T13:50:33Z

crates/store/src/db/sql.rs

+        ORDER BY
+            transaction_id ASC


I don't think any ordering would matter for the client, but it makes sense for reproducible results.

crates/block-producer/src/block_builder/prover/mod.rs

crates/store/src/state.rs

mFragaBA

One thing I noticed we might still have problems with in the client is that we're still not getting the info of when the transaction was included in a block. This is due to the fact that we return a sync state response with all nullifiers and transaction ids in the range (request.block_header, next_block_header) where block header is the block of the first note matching any of the requests' tags or account ids. So, if many TXs that only consume notes are sent and then we sync after a while, we'll still receive the TXs ids but won't know which got included in which block. A similar thing happens with nullifiers as well.

bobbinth · 2024-06-07T21:02:57Z

One thing I noticed we might still have problems with in the client is that we're still not getting the info of when the transaction was included in a block. This is due to the fact that we return a sync state response with all nullifiers and transaction ids in the range (request.block_header, next_block_header) where block header is the block of the first note matching any of the requests' tags or account ids. So, if many TXs that only consume notes are sent and then we sync after a while, we'll still receive the TXs ids but won't know which got included in which block. A similar thing happens with nullifiers as well.

Ah - good point! We could return (TransactionId, block_num) and (Nullifier, block_num) tuples. But that's separate from this PR. Could you create a new issue for this?

bobbinth

Looks good! Thank you!

Before merging this, let's do the changes I mentioned here (will require a PR in miden-base).

In a follow-up PR we can update the structure of returned data (i.e., move transaction IDs into account summaries, return block numbers for nullifiers and transaction IDs).

mFragaBA · 2024-06-07T21:08:40Z

also, I think after 0xPolygonMiden/miden-base#732 got merged in base it's breaking compilation, idk if we should fix it here or should I open a new PR (I still need these changes to compile the node)

bobbinth · 2024-06-08T08:14:51Z

also, I think after 0xPolygonMiden/miden-base#732 got merged in base it's breaking compilation, idk if we should fix it here or should I open a new PR (I still need these changes to compile the node)

Ah, yes! @polydez could you update this PR to make sure it works with latest next from miden-base? For example, the format_input_notes() function could look something like this:

pub fn format_input_notes(notes: &InputNotes<InputNoteCommitment>) -> String {
    format_array(notes.iter().map(|c| match c.note_id() {
        Some(note_id) => format!("({}, {})", c.nullifier().to_hex(), note_id.to_hex()),
        None => format!("({})", c.nullifier().to_hex()),
    }))
}

After this, we'll merge this PR and can address my comments from #377 (comment) in a separate PR.

mFragaBA · 2024-06-10T16:13:25Z

One thing I noticed we might still have problems with in the client is that we're still not getting the info of when the transaction was included in a block. This is due to the fact that we return a sync state response with all nullifiers and transaction ids in the range (request.block_header, next_block_header) where block header is the block of the first note matching any of the requests' tags or account ids. So, if many TXs that only consume notes are sent and then we sync after a while, we'll still receive the TXs ids but won't know which got included in which block. A similar thing happens with nullifiers as well.

Ah - good point! We could return (TransactionId, block_num) and (Nullifier, block_num) tuples. But that's separate from this PR. Could you create a new issue for this?

created #380. I'm open to taking it

bobbinth

Looks good! Thank you! I've pushed a couple of small commits to point this back at next in miden-base and also did some minor refactoring.

bobbinth · 2024-06-10T23:06:27Z

created #380. I'm open to taking it

Thank you! I'll assign it to you.

#377)

feat: return transaction IDs in state sync endpoint

6011290

polydez requested review from igamigo and bobbinth June 6, 2024 04:43

formatting: apply rustfmt

9ca2432

mFragaBA reviewed Jun 6, 2024

View reviewed changes

polydez added 2 commits June 7, 2024 10:32

refactor: update code for updates in miden-base

08a29ed

test: fix test_sql_select_transactions

37cbd78

bobbinth reviewed Jun 7, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

bobbinth reviewed Jun 7, 2024

View reviewed changes

crates/store/src/state.rs Outdated Show resolved Hide resolved

fix: address review comments

1943721

bobbinth requested changes Jun 7, 2024

View reviewed changes

crates/proto/proto/responses.proto Outdated Show resolved Hide resolved

crates/block-producer/src/block_builder/prover/mod.rs Outdated Show resolved Hide resolved

polydez added 3 commits June 7, 2024 17:01

fix: address review comments

2365346

Merge branch 'refs/heads/next' into polydez-tx-ids-in-sync

0ef876f

# Conflicts: # Cargo.lock # Cargo.toml # crates/store/src/state.rs

fix: clippy warnings

c904c9f

polydez force-pushed the polydez-tx-ids-in-sync branch from f5cf27a to c904c9f Compare June 7, 2024 12:04

polydez added 2 commits June 7, 2024 17:05

docs: update CHANGELOG.md

0805176

fix: check tx_hash on block applying

35c18ea

polydez requested a review from bobbinth June 7, 2024 12:24

igamigo approved these changes Jun 7, 2024

View reviewed changes

refactor: move compute_tx_hash to BlockWitness

7f79754

This was referenced Jun 7, 2024

fix: return error if partial note is found #379

Closed

Remove client dependency from faucet #368

Merged

mFragaBA reviewed Jun 7, 2024

View reviewed changes

bobbinth requested changes Jun 7, 2024

View reviewed changes

mFragaBA mentioned this pull request Jun 10, 2024

Return block height info along nullifiers and transaction ids #380

Closed

polydez added 3 commits June 10, 2024 21:46

refactor: migrate to the latest commit of miden-base

fbc3517

refactor: use Block::compute_tx_hash

99e6183

chore: update miden-base rev

f848602

polydez mentioned this pull request Jun 10, 2024

Refactored compute_tx_hash 0xPolygonMiden/miden-base#740

Merged

bobbinth added 3 commits June 10, 2024 15:00

chore: update miden-base dependencies to next branch

ffecc76

fix: use compute_tx_hash() function from miden-base

663127f

chore: update for the latest version of miden-base

afeac36

bobbinth approved these changes Jun 10, 2024

View reviewed changes

bobbinth merged commit 6e0689c into next Jun 10, 2024
6 checks passed

bobbinth deleted the polydez-tx-ids-in-sync branch June 10, 2024 23:05

bobbinth mentioned this pull request Jun 10, 2024

Return a list of committed transactions from the state sync endpoint #371

Closed

bobbinth pushed a commit that referenced this pull request Jun 10, 2024

feat: return the list of committed transactions in state sync endpoint (

3e67c2e

#377)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return the list of committed transactions in state sync endpoint #377

Return the list of committed transactions in state sync endpoint #377

polydez commented Jun 6, 2024 •

edited

Loading

mFragaBA left a comment

mFragaBA Jun 6, 2024

polydez Jun 7, 2024

mFragaBA Jun 6, 2024

polydez Jun 7, 2024

igamigo Jun 7, 2024 •

edited

Loading

mFragaBA Jun 6, 2024

polydez Jun 7, 2024 •

edited

Loading

bobbinth left a comment

bobbinth commented Jun 7, 2024

polydez commented Jun 7, 2024

igamigo left a comment •

edited

Loading

igamigo Jun 7, 2024 •

edited

Loading

mFragaBA left a comment

bobbinth commented Jun 7, 2024

bobbinth left a comment

mFragaBA commented Jun 7, 2024 •

edited

Loading

bobbinth commented Jun 8, 2024

mFragaBA commented Jun 10, 2024 •

edited

Loading

bobbinth left a comment

bobbinth commented Jun 10, 2024

Return the list of committed transactions in state sync endpoint #377

Return the list of committed transactions in state sync endpoint #377

Conversation

polydez commented Jun 6, 2024 • edited Loading

mFragaBA left a comment

Choose a reason for hiding this comment

mFragaBA Jun 6, 2024

Choose a reason for hiding this comment

polydez Jun 7, 2024

Choose a reason for hiding this comment

mFragaBA Jun 6, 2024

Choose a reason for hiding this comment

polydez Jun 7, 2024

Choose a reason for hiding this comment

igamigo Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

mFragaBA Jun 6, 2024

Choose a reason for hiding this comment

polydez Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth commented Jun 7, 2024

polydez commented Jun 7, 2024

igamigo left a comment • edited Loading

Choose a reason for hiding this comment

igamigo Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

mFragaBA left a comment

Choose a reason for hiding this comment

bobbinth commented Jun 7, 2024

bobbinth left a comment

Choose a reason for hiding this comment

mFragaBA commented Jun 7, 2024 • edited Loading

bobbinth commented Jun 8, 2024

mFragaBA commented Jun 10, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth commented Jun 10, 2024

polydez commented Jun 6, 2024 •

edited

Loading

igamigo Jun 7, 2024 •

edited

Loading

polydez Jun 7, 2024 •

edited

Loading

igamigo left a comment •

edited

Loading

igamigo Jun 7, 2024 •

edited

Loading

mFragaBA commented Jun 7, 2024 •

edited

Loading

mFragaBA commented Jun 10, 2024 •

edited

Loading