refactor: Introduce strictly typed and assuredly validated AccountId #4440

miraclx · 2021-06-30T15:30:31Z

This PR introduces a strictly typed structure - AccountId that enforces account ID validation.
This way, we can ensure validity and thereby fail fast before an invalid account name is used.

This also, optionally, ensures validation on {,de}serialization so, yeah..

The structure is defined within its own self-contained crate near-account-id to allow for easy publishing.
The extra dependencies - borsh, serde are made optional to allow the crate to be lightweight enough to be depended upon.

Within nearcore, the structure is reexported via near_primitives::AccountId and the crate via near_primitives{,_core}::account::id with default features (borsh, serde) but you can opt-out of those by depending on it directly.

Resolves #2074, supersedes #2831.

Testing Plan

Existing unit tests pass with minor updates
Nayduck tests pass without modifications
Manually ensured that the protocol is not changed

miraclx · 2021-06-30T17:39:00Z

Since we now preemptively perform account ID validation, cases like these are ensured and extra testing is unnecessarily redundant. However, this, in turn, leads to having variants like InvalidTxError::InvalidReceiverId that are unused:

nearcore/core/primitives/src/errors.rs

Line 119 in a6bf293

InvalidReceiverId { receiver_id: AccountId },

Would be nice if we could clean that up a bit, without breaking a lot.

miraclx · 2021-06-30T17:46:26Z

Also, throughout the repo, a debug view has been used on AccountId before this change. Back when it was simply a String this was okay.. But now, it's a lot more verbose (perhaps more descriptive, depending on how you look at it)

As an example:

format!("{:?}", AccountId("miraclx.near"))	before	`"miraclx.near"`
format!("{:?}", AccountId("miraclx.near"))	after	`"AccountId(\"miraclx.near\")"`

We could manually impl Debug and remain consistent with the former behavior but the proper Debug view seems more reasonable to me, what do you think?

I bring this up because it's one of the issues with failing tests in CI

snapshot (CI)

---- adapters::tests::test_convert_block_changes_to_transactions stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Differences ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: chain/rosetta-rpc/src/adapters/snapshots/near_rosetta_rpc__adapters__tests__validators_update_transaction.snap
Snapshot: validators_update_transaction
Source: chain/rosetta-rpc/src/adapters/mod.rs:992
-old snapshot
+new results
────────────────────────────────────────────────────────────────────────────────
validators_update_transaction
────────────┬───────────────────────────────────────────────────────────────────
   12    12 │             status: Some(
   13    13 │                 Success,
   14    14 │             ),
   15    15 │             account: AccountIdentifier {
-  16       │-                address: "nfvalidator1.near",
+        16 │+                address: AccountId(
+        17 │+                    "nfvalidator1.near",
+        18 │+                ),
   17    19 │                 sub_account: None,
   18    20 │             },
   19    21 │             amount: Some(
   20    22 │                 Amount {
┈┈┈┈┈┈┈┈┈┈┈┈┼┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
   37    39 │             status: Some(
   38    40 │                 Success,
   39    41 │             ),
   40    42 │             account: AccountIdentifier {
-  41       │-                address: "nfvalidator2.near",
+        43 │+                address: AccountId(
+        44 │+                    "nfvalidator2.near",
+        45 │+                ),
   42    46 │                 sub_account: None,
   43    47 │             },
   44    48 │             amount: Some(
   45    49 │                 Amount {
────────────┴───────────────────────────────────────────────────────────────────
thread 'adapters::tests::test_convert_block_changes_to_transactions' panicked at 'snapshot assertion for 'validators_update_transaction' failed in line 992', /var/lib/buildkite-agent/.cargo/registry/src/github.com-1ecc6299db9ec823/insta-1.6.3/src/runtime.rs:1026:9

miraclx · 2021-06-30T17:56:15Z

Also, node_key.json has a field account_id that previously held an empty string. CI fails for this exact reason. To mitigate this, this PR uses the "node" account id instead to ensure validity. This file has to be reset on CI systems.

cc @chefsale

Or should I engineer a workaround within this PR?

chefsale · 2021-06-30T18:35:26Z

So, the CI machines are destroyed on every run, so we don't store the keys, but rather generate them on every run. I believe the issue is rather caused by some code change where we have missed something. I assume this failure comes from account ids being invalid and this is throwing an error now? I assume there is some part of code which generates this node key which we need to adapt. cc: @bowenwang1996 @frol

Also just to verify we do not change the account id structure in the current generated key json files at all, but just have this additional check? If we do, we should ensure to be backwards compatible.

chefsale

Let's figure out how to fix the tests.

nearcore/src/config.rs

miraclx · 2021-07-28T12:55:14Z

Nayduck: http://nayduck.eastus.cloudapp.azure.com:3000/#/run/1915

ailisp · 2021-07-28T14:39:58Z

runtime/runtime/src/lib.rs

-
-        let n = 1;
-        let mut receipts = generate_receipts(small_transfer, n);
-        let invalid_account_id = "Invalid".to_string();


Cool these errors are statically prevented

ailisp · 2021-07-28T14:40:37Z

runtime/runtime/src/verifier.rs

-    }
+    // We retain these checks here as to maintain backwards compatibility
+    // with AccountId validation since we illegally parse an AccountId
+    // in near-vm-logic/logic.rs#fn(VMLogic::read_and_parse_account_id)


ailisp

Looks good. Nice hard and meticulous work!

chefsale

Good luck with resolving conflicts 🗡️

…countId (#4440)" This reverts commit a3805ee.

#4601) * Revert "refactor: Introduce strictly typed and assuredly validated AccountId (#4440)" This reverts commit a3805ee. * add missing tests Co-authored-by: Michal Nazarewicz <mina86@mina86.com>

…ear#4440) This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation. This way, we can ensure validity and thereby fail fast before an invalid account name is used. This also, optionally, ensures validation on {,de}serialization so, yeah.. The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing. The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon. Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly. Resolves near#2074, supersedes near#2831. # Testing Plan * [x] Existing unit tests pass with minor updates * [x] Nayduck tests pass without modifications * [x] Manually ensured that the protocol is not changed

…4440) This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation. This way, we can ensure validity and thereby fail fast before an invalid account name is used. This also, optionally, ensures validation on {,de}serialization so, yeah.. The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing. The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon. Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly. Resolves #2074, supersedes #2831. * [x] Existing unit tests pass with minor updates * [x] Nayduck tests pass without modifications * [x] Manually ensured that the protocol is not changed

# refactor: Introduce strictly typed and assuredly validated AccountId (#4440) This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation. This way, we can ensure validity and thereby fail fast before an invalid account name is used. This also, optionally, ensures validation on {,de}serialization so, yeah.. The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing. The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon. Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly. Resolves #2074, supersedes #2831. * [x] Existing unit tests pass with minor updates * [x] Nayduck tests pass without modifications * [x] Manually ensured that the protocol is not changed # impl workaround for invalid AccessKey-s in genesis records (resolves #4600) * [x] Manually booted a testnet node to ensure genesis loads properly and that no further AccountId-related issues occur. * [x] Ensured unit tests pass with no further updates * [x] Nayduck tests pass without modifications

Fixes the following clippy error: ``` error: the since field must contain a semver-compliant version --> core/account-id/src/lib.rs:302:18 | 302 | #[deprecated(since = "#4440", note = "AccountId construction without validation is illegal")] | ^^^^^^^^^^^^^^^ | = note: `#[deny(clippy::deprecated_semver)]` on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver ```

miraclx added 6 commits June 21, 2021 01:06

init

6ed44d7

init2

318768b

init3

ee97f6b

make borsh + serde optional, add tests for ser/de

e12ee32

Merge branch 'master' into strict-account-id-validity

2088a91

fix edge cases, remove tests for invalid ID

8b475ac

miraclx requested review from ailisp, artob, bowenwang1996, chefsale, EgorKulikov, frankbraun, frol, khorolets, matklad, mfornet, MaksymZavershynskyi, olonho and pmnoxx as code owners June 30, 2021 15:30

credits where credit's due

c975273

miraclx added A-security Area: Security issues A-testing Area: Unit testing / integration testing C-enhancement Category: An issue proposing an enhancement or a PR with one. C-housekeeping Category: Refactoring, cleanups, code quality labels Jun 30, 2021

chefsale suggested changes Jun 30, 2021

View reviewed changes

miraclx commented Jun 30, 2021

View reviewed changes

nearcore/src/config.rs Show resolved Hide resolved

miraclx added 3 commits July 28, 2021 12:27

fix is_implicit calls, liberate CI

7ba7b91

Merge branch 'master' into strict-account-id-validity

b8625ee

new_unchecked->new_unvalidated to be better descriptive, update docs

4a830d6

miraclx force-pushed the strict-account-id-validity branch from d2ac817 to 4a830d6 Compare July 28, 2021 11:42

khorolets approved these changes Jul 28, 2021

View reviewed changes

ailisp reviewed Jul 28, 2021

View reviewed changes

ailisp approved these changes Jul 28, 2021

View reviewed changes

bowenwang1996 approved these changes Jul 28, 2021

View reviewed changes

chefsale approved these changes Jul 28, 2021

View reviewed changes

miraclx requested a review from matklad July 28, 2021 19:54

mfornet approved these changes Jul 28, 2021

View reviewed changes

Merge branch 'master' into strict-account-id-validity

18f845f

matklad approved these changes Jul 29, 2021

View reviewed changes

frol changed the title ~~Introduce strictly typed and assuredly validated AccountId~~ refactor: Introduce strictly typed and assuredly validated AccountId Jul 29, 2021

miraclx added the S-automerge label Jul 29, 2021

Merge branch 'master' into strict-account-id-validity

dbf5597

near-bulldozer bot merged commit a3805ee into near:master Jul 29, 2021

miraclx deleted the strict-account-id-validity branch July 29, 2021 14:08

bowenwang1996 mentioned this pull request Jul 29, 2021

Account Id change caused neard to break on testnet #4600

Closed

bowenwang1996 added a commit that referenced this pull request Jul 29, 2021

Revert "refactor: Introduce strictly typed and assuredly validated Ac…

f7ef306

…countId (#4440)" This reverts commit a3805ee.

bowenwang1996 mentioned this pull request Jul 29, 2021

Revert "refactor: Introduce strictly typed and assuredly validated Ac… #4601

Merged

frol mentioned this pull request Jul 30, 2021

Automate canary node testing from PR #4607

Closed

miraclx mentioned this pull request Aug 2, 2021

refactor: Reintroduce strictly typed AccountId #4621

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Introduce strictly typed and assuredly validated AccountId #4440

refactor: Introduce strictly typed and assuredly validated AccountId #4440

miraclx commented Jun 30, 2021 •

edited by frol

Loading

miraclx commented Jun 30, 2021

miraclx commented Jun 30, 2021

miraclx commented Jun 30, 2021

chefsale commented Jun 30, 2021

chefsale left a comment

miraclx commented Jul 28, 2021

ailisp Jul 28, 2021

ailisp Jul 28, 2021

ailisp left a comment

chefsale left a comment

refactor: Introduce strictly typed and assuredly validated AccountId #4440

refactor: Introduce strictly typed and assuredly validated AccountId #4440

Conversation

miraclx commented Jun 30, 2021 • edited by frol Loading

Testing Plan

miraclx commented Jun 30, 2021

miraclx commented Jun 30, 2021

miraclx commented Jun 30, 2021

chefsale commented Jun 30, 2021

chefsale left a comment

Choose a reason for hiding this comment

miraclx commented Jul 28, 2021

ailisp Jul 28, 2021

Choose a reason for hiding this comment

ailisp Jul 28, 2021

Choose a reason for hiding this comment

ailisp left a comment

Choose a reason for hiding this comment

chefsale left a comment

Choose a reason for hiding this comment

miraclx commented Jun 30, 2021 •

edited by frol

Loading