Add support for on-chain `notes` to the database #300

phklive · 2024-04-05T15:02:20Z

In this PR I propose an update to the database to provide support for on-chain notes.

Closes: #288

+ add dabase to gitignore

igamigo · 2024-04-06T19:11:25Z

proto/proto/note.proto

@@ -12,6 +12,7 @@ message Note {
    account.AccountId sender  = 4;
    fixed64 tag = 5;
    merkle.MerklePath merkle_path = 7;
+    optional bytes details = 8;


Can we document somewhere what details is meant to contain and when the endpoint will return it?

rpc/README.md

igamigo · 2024-04-06T19:14:41Z

Do we want to base this PR off of phklive-get-notes-by-id?

Does that bring any issues?

These PR's are very linked. And the first PR is needed to correctly test this one.

I might be missing something but it seems this PR includes some changes that are also on the PR for phklive-get-notes-by-id, which makes it a a bit harder to tell which changes belong to this PR as opposed to the other.

phklive · 2024-04-06T21:20:21Z

Do we want to base this PR off of phklive-get-notes-by-id?

Does that bring any issues?
These PR's are very linked. And the first PR is needed to correctly test this one.

I might be missing something but it seems this PR includes some changes that are also on the PR for phklive-get-notes-by-id, which makes it a a bit harder to tell which changes belong to this PR as opposed to the other.

We had a misunderstanding, you were right indeed.

I re-based the PR on phklive-get-notes-by-id and did the same for this PR: #303

bobbinth

Thank you! Looks good! I left a few comments inline. The man one is about potentially making the details column nullable.

node/miden-node.toml

store/src/db/migrations.rs

bobbinth · 2024-04-07T08:07:20Z

store/src/db/sql.rs

            note_hash,
            sender,
            tag,
-            merkle_path
+            merkle_path,
+            details


This is probably OK for now, but in the future we should refactor this. The main reason is that select_notes_since_block_by_tag_and_sender() is used to build a sync_state response and for this response we don't need not details. With this approach, we'd be doing a lot of unnecessary work (e.g., reading the details form the database, parsing them etc.).

Let's create an issue to address this in the future.

Should I just remove the details field from this query ?

@phklive in public accounts we had the same issue and I removed this field from query and structure. I also added structure for specific endpoint for public accounts where this field is useful.

To the extent of my understanding as of now I do not think that it would be wise to simply remove the details field and set all note details to None this would be misleading. Hence I prefer to open a separate issue to refactor this flow.

Before opening the issue @bobbinth could you please clarify the reason why we would not need note details when syncing the state? I thought that we would also want to collect some new notes from the network using sync_state and that some of these notes could happen to be on-chain.

To the extent of my understanding as of now I do not think that it would be wise to simply remove the details field and set all note details to None this would be misleading. Hence I prefer to open a separate issue to refactor this flow.

Agreed. We should have a separate struct which is like note but without details (that's how it works for accounts, I believe). And yes, we can do it in a separate PR.

Before opening the issue @bobbinth could you please clarify the reason why we would not need note details when syncing the state? I thought that we would also want to collect some new notes from the network using sync_state and that some of these notes could happen to be on-chain.

State sync could return many irrelevant notes (because we do a kind of "fuzzy" matching based on tags) and sending details for such notes could be expensive. So, the process is that with state sync we get minimal note info (Id + metadata + inclusion proofs) and if some of the notes happen to be on-chain, we can request details for them separately via the GetNotesById endpoint (this is actually one of the main reasons why we add this endpoint).

Opened issue here: #309

Regarding this, with the current information we get from the sync, there is no way to tell whether the note is on chain or not, so we should add that information to the response. @phklive @bobbinth @polydez

Does it not come with note metadata? If it is not there yet, we should make sure the protobuf messages include note_type.

Does it not come with note metadata? If it is not there yet, we should make sure the protobuf messages include note_type.

It comes with note metadata, but response of this endpoint doesn't contain note metadata.

Correct, the sync endpoint specifically does not include the note type in any form.

proto/src/domain/notes.rs

store/src/db/sql.rs

bobbinth · 2024-04-07T08:15:44Z

store/src/db/sql.rs

+        let details_data = row.get_ref(7)?.as_blob()?;
+        let details = deserialize(details_data)?;


Related to the above comment: if we make column nullable and store NULL on None, we might need to handle this slightly differently.

Does that only apply to notes or does it also apply to accounts? I have the understanding that we are not following this pattern when inserting accounts:

miden-node/store/src/db/sql.rs

Lines 159 to 214 in f5a7bd9

pub fn upsert_accounts(

transaction: &Transaction,

accounts: &[AccountUpdateDetails],

block_num: BlockNumber,

) -> Result<usize> {

let mut upsert_stmt = transaction.prepare(

"INSERT OR REPLACE INTO accounts (account_id, account_hash, block_num, details) VALUES (?1, ?2, ?3, ?4);",

)?;

let mut select_details_stmt =

transaction.prepare("SELECT details FROM accounts WHERE account_id = ?1;")?;

let mut count = 0;

for update in accounts.iter() {

let account_id = update.account_id.into();

let full_account = match &update.details {

None => None,

Some(AccountDetails::Full(account)) => {

debug_assert!(account.is_new());

debug_assert_eq!(account_id, u64::from(account.id()));

if account.hash() != update.final_state_hash {

return Err(DatabaseError::ApplyBlockFailedAccountHashesMismatch {

calculated: account.hash(),

expected: update.final_state_hash,

});

}

Some(Cow::Borrowed(account))

},

Some(AccountDetails::Delta(delta)) => {

let mut rows = select_details_stmt.query(params![u64_to_value(account_id)])?;

let Some(row) = rows.next()? else {

return Err(DatabaseError::AccountNotFoundInDb(account_id));

};

let account =

apply_delta(account_id, &row.get_ref(0)?, delta, &update.final_state_hash)?;

Some(Cow::Owned(account))

},

};

let inserted = upsert_stmt.execute(params![

u64_to_value(account_id),

update.final_state_hash.to_bytes(),

block_num,

full_account.as_ref().map(|account| account.to_bytes()),

])?;

debug_assert_eq!(inserted, 1);

count += inserted;

}

Ok(count)

}

@phklive if I understood you correctly, in line 205 I process None details as NULL.

@phklive if I understood you correctly, in line 205 I process None details as NULL.

Could you clarify, how do you convert the full_account to Null in case there are no details? It is not fully clear to me in line 205.

Could you also please clarify why you are using Cow::Borrowed and Cow::Owned?

@polydez

how do you convert the full_account to Null in case there are no details?

full_account is actually an Option here. Line 205 only translates account details in Some(...) container to binary, so it becomes Some(Vec<u8>) or None if details is None.

Could you also please clarify why you are using Cow::Borrowed and Cow::Owned?

It helps to provide owned value, as well, as borrowed. You can see that this match in line 173 can return borrowed account in a case it was provided by transaction or load account from the database, then apply delta and return new owned account.

bobbinth · 2024-04-09T20:47:06Z

@phklive - how close are we to getting this reviewed/merged?

phklive · 2024-04-09T21:58:38Z

@phklive - how close are we to getting this reviewed/merged?

Should be ready for review, finalized in the plane. Just arrived safely in Athens 🇬🇷

bobbinth

Looks good! Thank you!

Let's make sure to create an issue to remove note details from state sync responses.

Dominik1999 · 2024-04-10T07:22:54Z

Can we merge?

phklive · 2024-04-10T07:36:22Z

Can we merge?

Yes we can. Opening the issue now.

* Getting notes by id works * lint and test * Improved comments + Added conversion from digest::Digest to NoteId * Added rpc validation for NoteId's * Added documentation to Readmes * Lint * Added details field to database * Fixed ci problems * Added details to Note type * Added test for endpoint + refactored sql query + improved documentation * Fixed lint * Fixed ci with test problem * lint * Fix nit in migration + add dabase to gitignore * Order rpc and store endpoints alphabitically * Change name to database to prevent gitignore problems * generated * Changed serialized None to Null + added test * Added documentation * chore: force refresh of readme files * chore: remove duplicate GetNoteById descriptions from readme files --------- Co-authored-by: Bobbin Threadbare <bobbinth@protonmail.com>

phklive and others added 19 commits April 4, 2024 11:11

Getting notes by id works

7c09397

lint and test

24c05ab

Improved comments + Added conversion from digest::Digest to NoteId

c52dafe

Added rpc validation for NoteId's

626cb54

Added documentation to Readmes

0d80b8b

Lint

0335339

Merge next + lint

1a4a829

Added details field to database

0c25238

Fixed ci problems

e0572ba

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

9fd74e3

Added details to Note type

443795e

Added test for endpoint + refactored sql query + improved documentation

a6bd2ea

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

8ce4963

Fixed lint

a32b312

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

2b037a2

Fixed ci with test problem

895a7a2

lint

7907be9

Fix nit in migration

9daef27

+ add dabase to gitignore

Merge branch 'next' into phklive-db-onchain-note

fd34c43

phklive mentioned this pull request Apr 5, 2024

Implement get_notes_by_id endpoint #298

Merged

phklive added 5 commits April 5, 2024 20:46

Order rpc and store endpoints alphabitically

f9b4a0b

Change name to database to prevent gitignore problems

f0e7e1a

Change name to database to prevent gitignore problems

544473d

Merge branch 'next' into phklive-get-notes-by-id

3c5a9fd

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

f4ec7e5

phklive marked this pull request as ready for review April 5, 2024 20:02

phklive requested review from bobbinth and hackaugusto and removed request for bobbinth and hackaugusto April 5, 2024 20:02

phklive requested review from hackaugusto, igamigo, bobbinth and polydez and removed request for hackaugusto and igamigo April 6, 2024 00:18

igamigo reviewed Apr 6, 2024

View reviewed changes

phklive changed the base branch from next to phklive-get-notes-by-id April 6, 2024 21:18

bobbinth requested changes Apr 7, 2024

View reviewed changes

Base automatically changed from phklive-get-notes-by-id to next April 8, 2024 19:51

phklive added 3 commits April 9, 2024 10:53

Merged next + lint

cc4eeff

Merge branch 'next' into phklive-db-onchain-note

5b9db4e

Changed serialized None to Null + added test

12beded

Added documentation

c65e320

phklive requested review from bobbinth and igamigo April 9, 2024 22:15

bobbinth added 2 commits April 9, 2024 16:11

chore: force refresh of readme files

d69d144

chore: remove duplicate GetNoteById descriptions from readme files

3979987

bobbinth approved these changes Apr 9, 2024

View reviewed changes

phklive mentioned this pull request Apr 10, 2024

[Feature]: Improve implementation of get_state_sync removing details #309

Closed

phklive merged commit bae6864 into next Apr 10, 2024
5 checks passed

phklive deleted the phklive-db-onchain-note branch April 10, 2024 07:39

Dominik1999 mentioned this pull request Apr 10, 2024

Update storage to support on-chain notes #288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for on-chain `notes` to the database #300

Add support for on-chain `notes` to the database #300

phklive commented Apr 5, 2024 •

edited

Loading

igamigo Apr 6, 2024

igamigo commented Apr 6, 2024

phklive commented Apr 6, 2024

bobbinth left a comment

bobbinth Apr 7, 2024

phklive Apr 8, 2024

polydez Apr 9, 2024

phklive Apr 9, 2024

bobbinth Apr 9, 2024

phklive Apr 10, 2024

igamigo Apr 10, 2024 •

edited

Loading

bobbinth Apr 10, 2024

polydez Apr 10, 2024

igamigo Apr 10, 2024

bobbinth Apr 7, 2024

phklive Apr 9, 2024

polydez Apr 9, 2024 •

edited

Loading

phklive Apr 9, 2024 •

edited

Loading

polydez Apr 9, 2024

bobbinth commented Apr 9, 2024

phklive commented Apr 9, 2024 •

edited

Loading

bobbinth left a comment

Dominik1999 commented Apr 10, 2024

phklive commented Apr 10, 2024

		let details_data = row.get_ref(7)?.as_blob()?;
		let details = deserialize(details_data)?;

	pub fn upsert_accounts(
	transaction: &Transaction,
	accounts: &[AccountUpdateDetails],
	block_num: BlockNumber,
	) -> Result<usize> {
	let mut upsert_stmt = transaction.prepare(
	"INSERT OR REPLACE INTO accounts (account_id, account_hash, block_num, details) VALUES (?1, ?2, ?3, ?4);",
	)?;
	let mut select_details_stmt =
	transaction.prepare("SELECT details FROM accounts WHERE account_id = ?1;")?;

	let mut count = 0;
	for update in accounts.iter() {
	let account_id = update.account_id.into();
	let full_account = match &update.details {
	None => None,
	Some(AccountDetails::Full(account)) => {
	debug_assert!(account.is_new());
	debug_assert_eq!(account_id, u64::from(account.id()));

	if account.hash() != update.final_state_hash {
	return Err(DatabaseError::ApplyBlockFailedAccountHashesMismatch {
	calculated: account.hash(),
	expected: update.final_state_hash,
	});
	}

	Some(Cow::Borrowed(account))
	},
	Some(AccountDetails::Delta(delta)) => {
	let mut rows = select_details_stmt.query(params![u64_to_value(account_id)])?;
	let Some(row) = rows.next()? else {
	return Err(DatabaseError::AccountNotFoundInDb(account_id));
	};

	let account =
	apply_delta(account_id, &row.get_ref(0)?, delta, &update.final_state_hash)?;

	Some(Cow::Owned(account))
	},
	};

	let inserted = upsert_stmt.execute(params![
	u64_to_value(account_id),
	update.final_state_hash.to_bytes(),
	block_num,
	full_account.as_ref().map(\|account\| account.to_bytes()),
	])?;

	debug_assert_eq!(inserted, 1);

	count += inserted;
	}

	Ok(count)
	}

Add support for on-chain notes to the database #300

Add support for on-chain notes to the database #300

Conversation

phklive commented Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

igamigo commented Apr 6, 2024

phklive commented Apr 6, 2024

bobbinth 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Apr 10, 2024 • edited Loading

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

Choose a reason for hiding this comment

polydez Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

phklive Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented Apr 9, 2024

phklive commented Apr 9, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

Dominik1999 commented Apr 10, 2024

phklive commented Apr 10, 2024

Add support for on-chain `notes` to the database #300

Add support for on-chain `notes` to the database #300

phklive commented Apr 5, 2024 •

edited

Loading

igamigo Apr 10, 2024 •

edited

Loading

polydez Apr 9, 2024 •

edited

Loading

phklive Apr 9, 2024 •

edited

Loading

phklive commented Apr 9, 2024 •

edited

Loading