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

update rust to 1.60, solana to 1.11.6#3492

Merged
2501babe merged 12 commits intosolana-labs:masterfrom
2501babe:version-11.6
Aug 18, 2022
Merged

update rust to 1.60, solana to 1.11.6#3492
2501babe merged 12 commits intosolana-labs:masterfrom
2501babe:version-11.6

Conversation

@2501babe
Copy link
Copy Markdown
Contributor

@2501babe 2501babe commented Aug 17, 2022

i marked two things in the stake client as #[allow(deprecated)] because after looking at the relevant types in solana-client i dont understand what the correct change is

warning: use of deprecated field `solana_client::rpc_filter::Memcmp::encoding`: Field has no server-side effect. Specify encoding with `MemcmpEncodedBytes` variant instead.
  --> stake-pool/cli/src/client.rs:91:21
   |
91 |                     encoding: None,
   |                     ^^^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: use of deprecated field `solana_client::rpc_filter::Memcmp::encoding`: Field has no server-side effect. Specify encoding with `MemcmpEncodedBytes` variant instead.
   --> stake-pool/cli/src/client.rs:136:21
    |
136 |                     encoding: Some(MemcmpEncoding::Binary),
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

if possible i wanna merge this as is and worry about that later tho

Comment thread stake-pool/cli/src/client.rs Outdated
Comment thread stake-pool/cli/src/client.rs Outdated
@2501babe 2501babe requested a review from joncinque August 17, 2022 21:11
joncinque
joncinque previously approved these changes Aug 17, 2022
Copy link
Copy Markdown
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks good! Could you just add an issue to investigate and fix those #[allow(deprecated)]s? Might even be a possible "good-first-issue"

@CriesofCarrots
Copy link
Copy Markdown
Contributor

CriesofCarrots commented Aug 17, 2022

i dont understand what the correct change is

(Sorry for the flyby) The correct change should be to use a constructor fn for Memcmp: https://github.com/solana-labs/solana/blob/225cddcffbf453cc05f5c6ca2da74509a3fcde07/client/src/rpc_filter.rs#L147-L161
At a glance, the first should probably be Memcmp::new_raw_bytes(), the second Memcmp::new_base58_encoded(), although I can take closer look if helpful.
Also, just occurring to me we don't have a constructor for base64 encoding, so that should probably be added upstream.

@2501babe
Copy link
Copy Markdown
Contributor Author

re: the failures, it looks like the flags for one of the upstream tools (either cargo-test-bpf or cargo-test-sbf) have changed... or, rather, the flags that one of them generates to pass to something else are being rejected by the recipient because perhaps the call chain has changed?

    Finished test [unoptimized + debuginfo] target(s) in 5m 29s
     Running unittests (/home/runner/work/solana-program-library/solana-program-library/target/debug/deps/spl_feature_proposal-2570cef13791f5fb)
error: Unrecognized option: 'arch'
error: test failed, to rerun pass '--lib'
cargo-test-sbf child: /home/runner/.local/share/solana/install/active_release/bin/cargo-build-sbf --manifest-path /home/runner/work/solana-program-library/solana-program-library/feature-proposal/program/Cargo.toml --sbf-out-dir /home/runner/work/solana-program-library/solana-program-library/target/deploy --arch sbf
cargo-test-sbf child: /home/runner/.rustup/toolchains/1.60.0-x86_64-unknown-linux-gnu/bin/cargo test --manifest-path /home/runner/work/solana-program-library/solana-program-library/feature-proposal/program/Cargo.toml --features test-bpf -- --nocapture --arch bpf

Error: Process completed with exit code 1.

@dmakarov do you have any insight into this?

@dmakarov
Copy link
Copy Markdown
Contributor

re: the failures, it looks like the flags for one of the upstream tools (either cargo-test-bpf or cargo-test-sbf) have changed... or, rather, the flags that one of them generates to pass to something else are being rejected by the recipient because perhaps the call chain has changed?

    Finished test [unoptimized + debuginfo] target(s) in 5m 29s
     Running unittests (/home/runner/work/solana-program-library/solana-program-library/target/debug/deps/spl_feature_proposal-2570cef13791f5fb)
error: Unrecognized option: 'arch'
error: test failed, to rerun pass '--lib'
cargo-test-sbf child: /home/runner/.local/share/solana/install/active_release/bin/cargo-build-sbf --manifest-path /home/runner/work/solana-program-library/solana-program-library/feature-proposal/program/Cargo.toml --sbf-out-dir /home/runner/work/solana-program-library/solana-program-library/target/deploy --arch sbf
cargo-test-sbf child: /home/runner/.rustup/toolchains/1.60.0-x86_64-unknown-linux-gnu/bin/cargo test --manifest-path /home/runner/work/solana-program-library/solana-program-library/feature-proposal/program/Cargo.toml --features test-bpf -- --nocapture --arch bpf

Error: Process completed with exit code 1.

@dmakarov do you have any insight into this?

I don't know how cargo-test-sbf was started. Any options that appear on cargo test command line after --features test-bpf were added to cargo-test-sbf command line after --. cargo-test-sbf simply forwards these options to cargo test command line. Naturally test harness rejects --arch command line option.

@mergify mergify Bot dismissed joncinque’s stale review August 17, 2022 22:39

Pull request has been modified.

Comment thread stake-pool/cli/src/client.rs Outdated
Comment thread stake-pool/cli/src/client.rs
@2501babe
Copy link
Copy Markdown
Contributor Author

the issue with --arch bpf is something in the sdk utils is tacking it on at the end in 1.16 where this didnt happen in 1.15. per jon i tried switching from test-bpf calls to test-sbf calls, which doesnt have this wrapper layer

i think we will still have a few test failures now (im not sure how to run these job on my system and the tests pass when invoked normally so i have to rely on ci) which could be 1.16 changes still to triage

@dmakarov
Copy link
Copy Markdown
Contributor

dmakarov commented Aug 17, 2022

the issue with --arch bpf is something in the sdk utils is tacking it on at the end in 1.16 where this didnt happen in 1.15. per jon i tried switching from test-bpf calls to test-sbf calls, which doesnt have this wrapper layer

@joncinque just fixed this in #27221

(im not sure how to run these job on my system and the tests pass when invoked normally so i have to rely on ci)

Could you run the CI script locally?

./ci/cargo-test-bpf.sh token

Comment thread ci/cargo-test-bpf.sh Outdated
set -x
cd $run_dir
cargo +"$rust_stable" test-sbf -- --nocapture
cargo +"$rust_stable" test-sbf --arch bpf -- --nocapture
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need to specify bpf architecture. It's the default. The option was added to test sbfv2 architecture as means to test the changes in the new architecture.

@2501babe
Copy link
Copy Markdown
Contributor Author

after fighting this awhile the two outstanding issues are:

  • token tests fail when --arch bpf is removed from the ci script. after going through logs, it looks like the reason this seemed to fix the failures is because it causes the tests that happen to fail to not run at all. i dont know why
  • stake pool python client tests fail. i thought this was due to my change to the stake pool cli, but it still fails after reverting it. possibly other things changed with the filters, possibly unrelated. ive been unable to run the python stuff locally

ill try again tomorrow

@2501babe 2501babe marked this pull request as draft August 18, 2022 01:24
@dmakarov
Copy link
Copy Markdown
Contributor

after fighting this awhile the two outstanding issues are:

* token tests fail when `--arch bpf` is removed from the ci script. after going through logs, it looks like the reason this seemed to fix the failures is because it causes the tests that happen to fail to not run at all. i dont know why

* stake pool python client tests fail. i thought this was due to my change to the stake pool cli, but it still fails after reverting it. possibly other things changed with the filters, possibly unrelated. ive been unable to run the python stuff locally

ill try again tomorrow

Adding --arch bpf enables these functions to be compiled

#[cfg(target_arch = "bpf")]
fn delete_account(account_info: &AccountInfo) -> Result<(), ProgramError> {
account_info.assign(&system_program::id());
account_info.realloc(0, false)
}
and
#[cfg(target_arch = "bpf")]
fn delete_account(account_info: &AccountInfo) -> Result<(), ProgramError> {
account_info.assign(&system_program::id());
account_info.realloc(0, false)
}

These #cfg[] annotations should be changed to #[cfg(target_os = "solana")], similarly #[cfg(not(target_arch = "bpf"))] should be changed to #[cfg(not(target_os = "solana"))]. Then cargo-build-sbf will do the right thing without the option --arch bpf.

@joncinque
Copy link
Copy Markdown
Contributor

These #cfg[] annotations should be changed to #[cfg(target_os = "solana")], similarly #[cfg(not(target_arch = "bpf"))] should be changed to #[cfg(not(target_os = "solana"))]. Then cargo-build-sbf will do the right thing without the option --arch bpf.

That's my fault, I made those changes in a security advisory before we had target_os and didn't think to update them when I finally merged it. I put in #3493 to address that bit for you

@joncinque
Copy link
Copy Markdown
Contributor

@2501babe let me look into the stake pool failures, it might be due to the minimum stake delegation

@joncinque
Copy link
Copy Markdown
Contributor

I fixed up the tests, rebased, and reverted your revert of the change, since you did it correctly. I ran a little test against testnet and it was perfect

@joncinque
Copy link
Copy Markdown
Contributor

I decided to just go the whole way and remove "bpf" everywhere in the code, so our features and everything are aligned now

@joncinque joncinque marked this pull request as ready for review August 18, 2022 14:05
# With minimum delegation at MINIMUM_DELEGATION + rent-exemption, when
# decreasing, we'll need rent exemption + minimum delegation delegated to
# cover all movements
increase_amount = MINIMUM_ACTIVE_STAKE + stake_rent_exemption
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ohhh it was the 1.11 change that everything needs to be rent exempt, i understand now

@2501babe 2501babe merged commit 65769ce into solana-labs:master Aug 18, 2022
@2501babe 2501babe deleted the version-11.6 branch August 18, 2022 19:16
@2501babe
Copy link
Copy Markdown
Contributor Author

thank you everyone for your help!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants