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

feat: protocol schema check #11749

Merged
merged 21 commits into from
Jul 12, 2024
Merged

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Jul 9, 2024

For all structs with derived ProtocolStruct, ProtocolStructInfo is created which defines a schema by underlying field names.
All ProtocolStructInfo creations are registered by inventory crate.
Script uses compute_hash to recursively compute hash of protocol struct, which should stay the same if borsh-serialization format didn't change.

Run: cargo +nightly run -p protocol-schema-check
Checked on cargo build -p near-primitives that only 2 more crates are introduced which are near empty.

Hashes are expected to stay stable now.

Cargo.lock Outdated Show resolved Hide resolved
protocol_structs.json Outdated Show resolved Hide resolved
tools/protocol-schema-check/src/main.rs Show resolved Hide resolved
tools/protocol-schema-check/src/main.rs Outdated Show resolved Hide resolved
core/structs-checker/src/lib.rs Outdated Show resolved Hide resolved
github-merge-queue bot pushed a commit that referenced this pull request Jul 9, 2024
Run `cargo update` for couple crates, allowing syn to bump to `2.0.70`.
Not sure if it is necessary, but cargo did it automatically for #11749.
@Longarithm Longarithm changed the title draft: protocol schema check v2 feat: protocol schema check v2 Jul 10, 2024
@Longarithm Longarithm marked this pull request as ready for review July 10, 2024 00:06
@Longarithm Longarithm requested a review from a team as a code owner July 10, 2024 00:06
@Longarithm Longarithm requested review from akhi3030 and nagisa and removed request for akhi3030 July 10, 2024 00:06
@Longarithm
Copy link
Member Author

Longarithm commented Jul 10, 2024

@nagisa now I'm a bit more serious about implementation, added readme, ensured that inventory doesn't leak to near-primitives without a feature. However, struct hashes seem unstable and I didn't guard const typeid well enough yet.

Copy link
Collaborator

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Overall the direction looks good to me, I identified a potential source of your hash instability. But I don't see anything fundamentally wrong that I'd object to, and the implementation seems pretty close to being ready to land.

tools/protocol-schema-check/src/main.rs Outdated Show resolved Hide resolved
tools/protocol-schema-check/src/main.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 8.82353% with 93 lines in your changes missing coverage. Please review.

Project coverage is 71.77%. Comparing base (0dc6336) to head (2867b42).
Report is 16 commits behind head on master.

Files Patch % Lines
tools/protocol-schema-check/src/main.rs 0.00% 82 Missing ⚠️
...re/structs-checker/structs-checker-core/src/lib.rs 0.00% 10 Missing ⚠️
core/primitives-core/src/account.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11749      +/-   ##
==========================================
- Coverage   71.81%   71.77%   -0.04%     
==========================================
  Files         792      796       +4     
  Lines      162765   162922     +157     
  Branches   162765   162922     +157     
==========================================
+ Hits       116884   116943      +59     
- Misses      40831    40921      +90     
- Partials     5050     5058       +8     
Flag Coverage Δ
backward-compatibility 0.23% <60.00%> (+<0.01%) ⬆️
db-migration 0.23% <60.00%> (+<0.01%) ⬆️
genesis-check 1.35% <70.00%> (+<0.01%) ⬆️
integration-tests 37.90% <8.82%> (-0.03%) ⬇️
linux 71.28% <7.84%> (-0.05%) ⬇️
linux-nightly 71.38% <8.82%> (-0.03%) ⬇️
macos 52.96% <7.84%> (-1.62%) ⬇️
pytests 1.58% <70.00%> (+<0.01%) ⬆️
sanity-checks 1.38% <70.00%> (+<0.01%) ⬆️
unittests 66.19% <8.82%> (-0.06%) ⬇️
upgradability 0.28% <60.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Longarithm Longarithm requested a review from nagisa July 10, 2024 19:03
@Longarithm
Copy link
Member Author

Longarithm commented Jul 10, 2024

Should pass the clippy and udeps checks now.

Also hashing TypeIds looks unstable as well. When I run it on mac and linux, hashes are different. Should I find some stable way to replace type ids with their equivalency classes and manually assign type ids to some primitive types? (integers, strings, vectors...)

core/primitives/Cargo.toml Outdated Show resolved Hide resolved
@nagisa
Copy link
Collaborator

nagisa commented Jul 11, 2024

When I run it on mac and linux, hashes are different. Should I find some stable way to replace type ids with their equivalency classes and manually assign type ids to some primitive types? (integers, strings, vectors...)

Hashes being different between different targets is unsurprising to me. In fact, Rust can only give a guarantee that they stay the same between compilations if the exact same code is compiled with the exact same compiler version.

Easy way out is to have your snapshot be per-platform and test on both platforms in CI. Or not and only support running this tool on x86_64-unknown-linux-gnu. That said that again raises a question of how does borsh behave when given e.g. usize. Will it transparently encode them the same way regardless of the target bitness? If not, then it may very well make sense to run this test on a 32-bit platform too. Even if it does, there's no real reason to test what's happening on a mac however, as that's not a platform we support running a validator on.

@Longarithm Longarithm requested a review from nagisa July 11, 2024 16:17
@Longarithm
Copy link
Member Author

Longarithm commented Jul 11, 2024

Addressed new suggestions.

I computed hashes on two different linux machines and still got mismatch for all structs though.

Hash mismatch for AccessKey: stored 4272749842, current 33301916
Hash mismatch for Account: stored 576458267, current 3788188897
Hash mismatch for AccountV2: stored 3397149883, current 2847141399
Hash mismatch for CryptoHash: stored 2136842504, current 1798747012
Hash mismatch for LegacyAccount: stored 472222693, current 252579903
Hash mismatch for ValidatorKickoutReason: stored 586085289, current 3237982902

Maybe I should use TypeId only for finding the right child structs to compute hash recursively?
Then bytes to hash will be concatenation of all field names, but it still implies the borsh safety in all reasonable cases.

UPD: Actually they had different nightly versions. When I upgraded nightly to 1.81, it stabilized everywhere except ValidatorKickoutReason. Maybe this is something specific about enums.

UPD 2: falling back to type names only.

@Longarithm Longarithm changed the title feat: protocol schema check v2 feat: protocol schema check Jul 11, 2024
@Longarithm Longarithm enabled auto-merge July 12, 2024 14:39
@Longarithm Longarithm added this pull request to the merge queue Jul 12, 2024
Merged via the queue into near:master with commit 9ba047b Jul 12, 2024
27 of 30 checks passed
@Longarithm Longarithm deleted the protocol-struct-3 branch July 12, 2024 15:35
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.

2 participants