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

Account Id change caused neard to break on testnet #4600

Closed
bowenwang1996 opened this issue Jul 29, 2021 · 4 comments · Fixed by #4621
Closed

Account Id change caused neard to break on testnet #4600

bowenwang1996 opened this issue Jul 29, 2021 · 4 comments · Fixed by #4621
Assignees
@miraclx
Labels
C-bug Category: This is a bug T-public-interfaces Team: issues relevant to the public interfaces team

Comments

@bowenwang1996
Copy link
Collaborator 

Jul 29 19:11:49 testnet-canary0 sh[11540]: thread 'main' panicked at 'error while streaming records: Custom { kind: InvalidData, error: Error("the value is too short for account ID", line: 1, column: 4955377026) }', /var/lib/buildkite-agent/builds/buildkite-i-0af3ccd2bfd2f721c-1/nearprotocol/nearcore-release/core/chain-configs/src/genesis_config.rs:422:18

To reproduce:
spin up a node on testnet with a commit that contains #4440 and observe that it produces the error above.
@bowenwang1996 bowenwang1996 added C-bug Category: This is a bug T-public-interfaces Team: issues relevant to the public interfaces team labels Jul 29, 2021
@bowenwang1996 bowenwang1996 mentioned this issue Jul 29, 2021
Merged
@frol
Copy link
Collaborator

frol commented Jul 30, 2021

@miraclx I feel like the issue is in the genesis records (genesis.json file) since it crashed right away, but it is super odd.

Jul 29 19:08:20 testnet-canary0 systemd[1]: Stopped Near Canary Service.
Jul 29 19:08:20 testnet-canary0 systemd[1]: Started Near Canary Service.
Jul 29 19:08:20 testnet-canary0 sh[11495]: Jul 29 19:08:20.126  INFO neard: Version: trunk, Build: f868aeef, Latest Protocol: 46
Jul 29 19:10:02 testnet-canary0 sh[11495]: thread 'main' panicked at 'error while streaming records: Custom { kind: InvalidData, error: Error("the value is too short for account ID", line: 1, column: 4955377026) }', /var/lib/buildkite-agent/builds/buildkite-i-0af3ccd2bfd2f721c-1/nearprotocol/nearcore-release/core/chain-configs/src/genesis_config.rs:422:18
Jul 29 19:10:02 testnet-canary0 sh[11495]: note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@bowenwang1996
Copy link
Collaborator Author

Indeed. It is possible that genesis contains an invalid account id for some reason

@miraclx
Copy link
Contributor

miraclx commented Jul 30, 2021 •
edited
Loading

Indeed it does (and not just one), here's one such example in .records[]

{
  "AccessKey": {
    "account_id": "figment.test2",
    "public_key": "ed25519:3KA1iGnyAa8ZWj1e27GxG38oW15BkrZF2PGQkCATJdxe",
    "access_key": {
      "nonce": 0,
      "permission": {
        "FunctionCall": {
          "allowance": "100000000",
          "receiver_id": "",
          "method_names": []
        }
      }
    }
  }
},

receiver_id is empty. Is this behavior even legal? If it is, we could maybe represent that field in an Option<AccountId>.

@bowenwang1996
Copy link
Collaborator Author

The behavior is technically legal but should not have been. Such access keys are useless -- any transactions signed by them will result in a InvalidAccessKeyError

This was referenced Aug 2, 2021
Merged
Closed
@frol frol mentioned this issue Aug 4, 2021
Closed
@frol frol closed this as completed in #4621 Aug 5, 2021
frol pushed a commit that referenced this issue Aug 5, 2021
@miraclx
refactor: Reintroduce strictly typed AccountId (#4621)
2d7b3c0 
# 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@miraclx miraclx

Labels
C-bug Category: This is a bug T-public-interfaces Team: issues relevant to the public interfaces team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: Reintroduce strictly typed AccountId miraclx/nearcore
3 participants
@frol @bowenwang1996 @miraclx