Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Introduce automatic ABI maintenance mechanism (2/2; rollout)#8012

Merged
ryoqun merged 8 commits intosolana-labs:masterfrom
ryoqun:abi-management-implementation
Jul 6, 2020
Merged

Introduce automatic ABI maintenance mechanism (2/2; rollout)#8012
ryoqun merged 8 commits intosolana-labs:masterfrom
ryoqun:abi-management-implementation

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Jan 29, 2020 •
edited
Loading

Problem

ABI can change without noticing.

Summary of Changes

This pr is currently only containing the actual rollout code to enable frozen abi. most are quite simple. But some are advanced. :) I'll hope this pr will become an example repository pr for future abi maintenance by our team mates. :)

This landed on #10335

This PR currently completely changed since this original implementation:

  • Introduce macro-based lexical ABI digest calculation and check mechanism deriving from the rust type system.
    • As noted by Propose Solana ABI management #7524, the detection mechanism isn't perfect; both false positives and negatives are possible. For example, pure type rename is a false positive. The ABI-affecting struct field's change is a false negative. Both are due to being lexical comparison nature of this implementation.

Now, this is the correct:

This PR approached the problem with serde's type system via custom Serializer for runtime inspection for more precise structural comparison of types.

todo

- [ ] backport to v0.23
- [ ] Once merged, add more frozen_abi to other top most types for serialization.

Was split to #10335

Fixes #7738

@ryoqun ryoqun added the work in progress This isn't quite right yet label Jan 29, 2020
@ryoqun ryoqun requested a review from mvines January 29, 2020 09:35
ryoqun
ryoqun commented Jan 29, 2020
View reviewed changes
Comment thread programs/vote/src/vote_instruction.rs Outdated
ryoqun
ryoqun commented Jan 29, 2020
View reviewed changes
Comment thread sdk/macro/src/lib.rs Outdated
ryoqun
ryoqun commented Jan 29, 2020
View reviewed changes
Comment thread sdk/macro/src/lib.rs Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 29, 2020 •
edited
Loading

Codecov Report

Merging #8012 into master will increase coverage by 0.1%.
The diff coverage is 97.5%.

@@           Coverage Diff            @@
##           master   #8012     +/-   ##
========================================
+ Coverage    81.9%   82.1%   +0.1%     
========================================
  Files         310     316      +6     
  Lines       71844   71960    +116     
========================================
+ Hits        58903   59095    +192     
+ Misses      12941   12865     -76

mvines
mvines reviewed Jan 29, 2020
View reviewed changes
Comment thread sdk/src/lib.rs Outdated
mvines
mvines reviewed Jan 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

direction looks great so far

Comment thread runtime/src/hard_forks.rs
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread ci/nits.sh Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread runtime/src/hard_forks.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/Cargo.toml Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/build.rs
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/macro-frozen-abi-dummy/Cargo.toml Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/macro-frozen-abi-dummy/src/lib.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/macro-frozen-abi/Cargo.toml
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/src/abi_digester.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/src/lib.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/src/lib.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/macro-frozen-abi/src/lib.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/macro-frozen-abi/src/lib.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/src/abi_digester.rs Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread runtime/Cargo.toml Outdated
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread runtime/Cargo.toml
ryoqun
ryoqun commented Feb 3, 2020
View reviewed changes
Comment thread sdk/src/lib.rs Outdated
@ryoqun ryoqun force-pushed the abi-management-implementation branch from b3b65b0 to 356a91c Compare February 4, 2020 14:55
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Feb 4, 2020

@mvines Status update!

Almost feature complete...
This is what frozen_abi can capture about the ABI for the Bank struct. Pretty impressive, imho:
https://gist.github.com/ryoqun/d0dea9e40b7898cbbe71eecef263c3c5 I think it's even ready for its own crate. ;)

Still, the main files are WAAAYY ugly. Don't even look at it.
Also, this PR alone got a bit too large. So, as a precaution, I'm keeping a patch set not a single commit for possible split into smaller PRs. On the other hand, I'm also thinking that this PR is ok as it is because this is technically test-code-only PR and its volume is dominated by the 2 completely new added files. Is there any preferred way for your easiest review life?

Let's start with easy documentations.

Currently I'm intentionally making CI red to show the ABI test is actually failing at the coverage job only, other jobs are completely not affected.

[note to be source-commented]:

The cfg matrix is rather complicated. There are now 3 independent conditions due to separate and unavoidable reasons...:

  • rustc version: stable vs nightly (the very impl of frozen_abi requires feature(specialization))
  • target: cfg(test) vs cfg(not(test)) (really constrain any frozen_abi impl under test to avert risks)
  • feature: feature = "program" vs not(feature = "program") (frozen_abi isn't needed by smart contracts)

And all of combinations (= 8) should be working now.

I've constructed rather complicated plumbing for my teammates to avoid cfg_attr(................, fronzen_abi(digest="...")) and simply write frozen_abi(digest = "...") and derive(AbiDigestSample).

Is there any thoughts in the cfg & usage aspects?

@ryoqun ryoqun force-pushed the abi-management-implementation branch from 446d117 to afe2ad5 Compare February 5, 2020 15:01
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Feb 7, 2020 •
edited
Loading

TODO: include bincode's serialized_size into the ABI digesting. Also might be useful for lightweight glance of memory foot prints like #8156

SKIP

@mvines mvines mentioned this pull request Feb 7, 2020
Merged
@ryoqun ryoqun mentioned this pull request Feb 10, 2020
Merged
@stale stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 16, 2020
@stale
Copy link
Copy Markdown

stale Bot commented Jun 25, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 25, 2020
@stale
Copy link
Copy Markdown

stale Bot commented Jul 2, 2020

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale Bot closed this Jul 2, 2020
@ryoqun ryoqun reopened this Jul 3, 2020
@stale stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 3, 2020
@ryoqun ryoqun force-pushed the abi-management-implementation branch from 5ca19c4 to 28a7c0e Compare July 3, 2020 07:13
ryoqun added 8 commits July 3, 2020 18:14
@ryoqun
Introduce automatic ABI maintenance mechanism (2/2; rollout)
2aa87dc
@ryoqun
Fix stable clippy
3e604d9
@ryoqun
Change to symlink
a6de98b
@ryoqun
Freeze abi of Tower
cc4a5f7
@ryoqun
fmt...
28b8af0
@ryoqun
Improve dev-experience!
d9369e8
@ryoqun
Update BankSlotDelta
aed8c47 
$ diff -u /tmp/abi8/*7dg6BreYxTuxiVz6aLvk3p2Z7GQk2cJqfGvC9h4FAoSj* /tmp/abi8/*9chBcbXVJ4fK7uGgydQzam5aHipaAKFw6V4LDFpjbE4w*
--- /tmp/abi8/bank__BankSlotDelta_frozen_abi__test_abi_digest_7dg6BreYxTuxiVz6aLvk3p2Z7GQk2cJqfGvC9h4FAoSj      2020-06-18 18:01:22.831228087 +0900
+++ /tmp/abi8/bank__BankSlotDelta_frozen_abi__test_abi_digest_9chBcbXVJ4fK7uGgydQzam5aHipaAKFw6V4LDFpjbE4w      2020-07-03 15:59:58.430695244 +0900
@@ -140,7 +140,7 @@
                                                         field u8
                                                             primitive u8
                                                         field solana_sdk::instruction::InstructionError
-                                                            enum InstructionError (variants = 34)
+                                                            enum InstructionError (variants = 35)
                                                                 variant(0) GenericError (unit)
                                                                 variant(1) InvalidArgument (unit)
                                                                 variant(2) InvalidInstructionData (unit)
@@ -176,6 +176,7 @@
                                                                 variant(31) CallDepth (unit)
                                                                 variant(32) MissingAccount (unit)
                                                                 variant(33) ReentrancyNotAllowed (unit)
+                                                                variant(34) MaxSeedLengthExceeded (unit)
                                                     variant(9) CallChainTooDeep (unit)
                                                     variant(10) MissingSignatureForFee (unit)
                                                     variant(11) InvalidAccountIndex (unit)
@ryoqun
Fix some merge conflicts...
0ad297e
@ryoqun ryoqun force-pushed the abi-management-implementation branch from 97fdf10 to 0ad297e Compare July 3, 2020 09:15
ryoqun
ryoqun commented Jul 3, 2020
View reviewed changes
Comment thread sdk/macro-frozen-abi/src/lib.rs
} else {
assert_eq!(#expected_digest, format!("{}", hash), "Possibly ABI changed? Confirm the diff by rerunning before and after this test failed with SOLANA_ABI_DUMP_DIR");
if let Ok(dir) = ::std::env::var("SOLANA_ABI_DUMP_DIR") {
assert_eq!(#expected_digest, actual_digest, "Possibly ABI changed? Examine the diff in SOLANA_ABI_DUMP_DIR!: $ diff -u {}/*{}* {}/*{}*", dir, #expected_digest, dir, actual_digest);
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Jul 3, 2020

Choose a reason for hiding this comment

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

@mvines I've made error message explicit how to proceed when encountering this. And, maintenance should be like this: aed8c47

ryoqun
ryoqun commented Jul 3, 2020
View reviewed changes
Comment thread runtime/src/bank.rs
pub const MAX_LEADER_SCHEDULE_STAKES: Epoch = 5;

type BankStatusCache = StatusCache<Result<()>>;
#[frozen_abi(digest = "9chBcbXVJ4fK7uGgydQzam5aHipaAKFw6V4LDFpjbE4w")]
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this digest is recently updated by #10748 (FYI: @mvines @jackcmay ) aed8c47

@ryoqun ryoqun requested review from mvines and removed request for mvines July 3, 2020 09:54
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Jul 3, 2020

@mvines I think this pr is ready for review and merge! I've rebased and there was little abi changes so far. So, it looks like there will be little churn for engineers to endure even after merging this. :)

mvines
mvines approved these changes Jul 6, 2020
View reviewed changes
Comment thread runtime/src/serde_snapshot/legacy.rs
}

#[cfg(all(test, RUSTC_WITH_SPECIALIZATION))]
impl IgnoreAsHelper for SerializableAccountStorageEntry {}
Copy link
Copy Markdown
Contributor

@mvines mvines Jul 6, 2020

Choose a reason for hiding this comment

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

It would be nice to have some brief documentation on when IgnoreAsHelper should be used, perhaps in
docs/src/implemented-proposals/abi-management.md

Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Jul 6, 2020

Choose a reason for hiding this comment

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

I see! I'll do this as a follow-up pr.

@ryoqun ryoqun merged commit 39b3ac6 into solana-labs:master Jul 6, 2020
This was referenced Jul 7, 2020
Merged
Merged
Merged
@ryoqun ryoqun mentioned this pull request Aug 11, 2020
Closed
ryoqun
ryoqun commented May 26, 2023
View reviewed changes
Comment thread ci/test-checks.sh
_ cargo +"$rust_stable" fmt --all -- --check

# -Z... is needed because of clippy bug: https://github.com/rust-lang/rust-clippy/issues/4612
# run nightly clippy for `sdk/` as there's a moderate amount of nightly-only code there
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun May 26, 2023

Choose a reason for hiding this comment

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

here: #31833

@ryoqun ryoqun mentioned this pull request May 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jackcmay jackcmay jackcmay left review comments

@mvines mvines mvines approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement Solana ABI management process

3 participants

@ryoqun @mvines @jackcmay