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.

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

objects: update account id scheme #672

Closed
wants to merge 33 commits into from
Closed

Conversation

hackaugusto
Copy link
Contributor

@hackaugusto hackaugusto commented May 7, 2024

closes #140

Opened the PR because the tests are green, but there is still some changes that are needed, namely the kernel is not checking the total PoW.

edit: well ... the tests that run with cargo test, turns out some tests are gated by the testing feature flag, which is great, because I thought we had no tests for the new accounts

bobbinth and others added 30 commits April 26, 2024 03:02
Change "In this second" to "In this "section"
* feat: adding account storage maps
…h tested code (#615)

* tests: unified test mods

* tests: moved account_id tests to its mod

* tests: moved account_code tests to its mod

* tests: using utils to test account data

* tests: using utils to test account

* tests: added serde tests for account and account delta

* add empty constructor for account storage delta

* added serde tests for storage delta

* tests using existing constants
* feat: add benchmark for default transaction

* refactor: use trace instead of emit, create wrapper for host

* refactor: improve cycles print

* refactor: add inline comments, change trace ids

* chore: ignore the test untill it become a binary

* refactor: organise cycles storing, add traces for each note

* feat: create a simple cli for benchmark binary package

* feat: create cargo-make script for benchmarking, move benchmarks back to the miden-tx

* refactor: move TransactionProgress to the TransactionHost

* feat: implement bench for P2ID note, add NoteId to the output

* refactor: move last note id obtaining to the separate function

* refactor: fix no-std build

* refactor: update comments in main.masm file to be consistent with api.masm

* feat: write benchmark results to the json file

* refactor: move benchmarks to their own crate

* refactor: add ability to run all benches

* refactor: update bin name, dependencies; remove benches except all

* chore: fix no-std build

* refactor: rework TransactionProgress serialization

* refactor: exclude bench-tx from no-std build, update dependencies

* refactor: improve imports formatting

* refactor: rework writing to json

* chore: change visibility of miden-tx MockDataStore

* chore: update CHANGELOG, create README for bench-tx
* process_input_note: simplified advice stack params names
* kernel: using stdlib utility to extract digest
* prologue: added details about nullifier
* tests: explain why Send/Sync is being enforced
* removed contraints on the struct definition

The type constraints should be added into the `impl`s blocks, because
the constraints on the `struct`s propagate up and makes composition very
verbose.

This also renamed the mod data to data_store to better represent its
content.
- Remove network execution, since currently this concept is not really
  used and resulted in a mismatch among the Rust and MASM code
- Changed the conversions to the NoteTag to include validation
- Fixed the NoteTag validation
- Fixed the tag validation in the `create_note` kernel code
- Fixed the tests to create valid note tags
Derive `Ord` and `PartialOrd` for `NoteTag`
* feat: Serialize auth secret key

* Remove nls

* Address reviews

* Remove allocation

* Rename var
* feat: optional initial hash for new accounts
fix: use `core::ops::Not` instead of `std::ops::Not`
fix: address review comments
refactor: rename function
docs: update changelog
tests: add checking proven transaction id for cases when account creation was involved

* refactor: move `init_account_hash` function back to `Account`

* fix: no-std build

* fix: address review comments
* refactor: move event emitting after validation

In this commit refactoring was made for account procedures and incr_nonce procedure. set_item and set_map_item will be reworked in future commits.

* refactor: move emit in the incr_nonce
* Rename NoteEnvelope into NoteHeader
* Change Note struct to be based on header and details
* feat: return NoteDetails for payback note from SWAP note constructor
* feat: properly construct tag for SWAP notes
@hackaugusto hackaugusto force-pushed the hacka-account-id-issue-140 branch 2 times, most recently from 89d9ef4 to c836688 Compare May 7, 2024 19:06
@hackaugusto
Copy link
Contributor Author

hackaugusto commented May 7, 2024

I pushed the latest version of the code. The tests are still not passing, I just realized that I'm computing the digest in the kernel using account id instead of account config, I also have to update the masm patching. Will follow up and hopefully fish this tomorrow.

* feat: implement support for full private notes

* docs: update CHANGELOG.md

* fix: address review comments
@hackaugusto hackaugusto force-pushed the hacka-account-id-issue-140 branch 5 times, most recently from a00e013 to 1f3364b Compare May 8, 2024 21:48
@hackaugusto hackaugusto marked this pull request as ready for review May 8, 2024 21:48
@hackaugusto hackaugusto requested a review from bobbinth May 8, 2024 21:49
#! - Account creation is not free, this introduces a small cost to account creation which as a small
#! spam proteciton.
#!
#! Stack: [CODE_ROOT, SEED, STORAGE_ROOT, acc_id]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the procedure to receive the data over the stack, so that it can be tested. This only has the cost of two additional instrucitons swapw and a movdnw.2

}
}

impl TryFrom<u64> for AccountPoW {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: These types are using u64 conversion to and from their mask position! This is confusing at times, I had a few instances that tried to PoW 80bits because I used u64::from(pow) instead of pow.as_int()

@hackaugusto hackaugusto force-pushed the hacka-account-id-issue-140 branch 3 times, most recently from f12ff3c to 5702fd3 Compare May 8, 2024 22:02
@hackaugusto
Copy link
Contributor Author

humm, it seems the tests are timing out in the CI, they are also taking longer time for me locally, around 1min to complete.

@hackaugusto hackaugusto force-pushed the hacka-account-id-issue-140 branch 3 times, most recently from fcac270 to 838f2c8 Compare May 8, 2024 22:43
- the account id now includes a configuration for the PoW, so users can
  increase its total PoW up to 64 bits. (Note: The total PoW required by
  an attacker includes the random bits of an account_id, so an
  additional 50 bits)
- the faucet and regular account ids are the same
- the lower to bitw of the account id are force to zero
- the storage and type configuration are in the 4 highest bits
@bobbinth
Copy link
Contributor

The schemes that we have been discussing (including the one implemented here) have a very serious flaw: the attacker could build a "rainbow table" of all (or many) account IDs and then as soon as a public note directed to a specific account appears on chain, they would use one of their pre-computed accounts to highjack the note.

Assuming there are $2^{60}$ possible account values, the storage required for such a rainbow table would be about $2^{64}$ bytes (8 bytes for ID + ~8 bytes for the seed). This is approximately 16M TB. Is is by no doubt a lot, but storing 1TB of data can cost as low as $10 USD/month. So, monthly storage costs of this rainbow table could be as low as $160M USD. What's worse, the attacker does not need to store the entire table - they could store just 1/1000 of it. The cost would be 160K/month, but effectively this would disrupt the network as any account would have a 1/1000 chance of being highjacked.

Building such a rainbow table (assuming some efficient GPU-based implementation) could cost anywhere between $1B and $10B USD. It is probably not something we'd worry about for the next couple of years, but beyond that, this would be a real problem.

Taken the above into account, I think ~64 bits for account ID is too little. So, one of the decisions we should make is what should be the ID size. A few preliminary thoughts:

  1. Something around ~100 bits is probably enough from the security standpoint, but we might as well go higher.
  2. 120 bits could be a good choice as it is high enough to be beyond any doubt and fits well into 2 field elements.
  3. 128 bits could also be an option as it cleanly fits into 2 field elements.

Increasing the size of the ID would have the following implications:

  1. We probably no longer need to encode PoW into the ID as even no PoW should be good enough to prevent account highjacking.
  2. We may again consider encoding ticker symbol into the ID (though, not sure if this is a good idea).
  3. Note metadata will need to be updated as now the sender_id will need to take up ~2 field elements.
  4. Structure of assets may need to be updated. For fungible assets this may not be an issue as we have 2 "free" elements, but not quite sure what to do with non-fungible assets. We may consider changing asset format to always be 2 words - but also not sure if that's a good idea.

@hackaugusto
Copy link
Contributor Author

closing this PR following the comment above

@hackaugusto hackaugusto deleted the hacka-account-id-issue-140 branch May 20, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants