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

chore: update rust to 1.82 #1587

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

chore: update rust to 1.82 #1587

wants to merge 6 commits into from

Conversation

Stebalien
Copy link
Member

And explicitly disable features we don't support.

@rvagg
Copy link
Member

rvagg commented Dec 3, 2024

I have a local version of this that I've rebased to current master and got the lints all sorted out but it still has a couple of test failures that are a bit concerning:

---- activate_deals_one_sector stdout ----
thread 'activate_deals_one_sector' panicked at runtime/src/test_utils.rs:1184:9:
assertion `left == right` failed: send to f07 expected method 3621052141, was 3261979605
  left: 3621052141
 right: 3261979605
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- sectors_fail_and_succeed_independently_during_batch_activation stdout ----
thread 'sectors_fail_and_succeed_independently_during_batch_activation' panicked at runtime/src/test_utils.rs:1184:9:
assertion `left == right` failed: send to f07 expected method 3621052141, was 3261979605
  left: 3621052141
 right: 3261979605

Why would the FRC42 method numbers be changed with this update? Aren't they supposed to be a stable hash value?

@Stebalien
Copy link
Member Author

They are supposed to be stable. I tested this because @raulk reported that 1.82 enables some wasm features we don't support in the FVM but... that shouldn't affect the tests here.

@Stebalien
Copy link
Member Author

Can you force-push your updated version?

And explicitly disable features we don't support.
@rvagg
Copy link
Member

rvagg commented Dec 4, 2024

done .. and done again to match the new master from yesterday

@Stebalien
Copy link
Member Author

Ah, ok. It's not computing the wrong number, it's calling the wrong function.

We're expecting a call to Balance on f07, but we're getting a call to TransferFrom. And I think I found the bug.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

oooops .. is_none, my bad

@@ -291,7 +291,7 @@ pub fn call_generic<RT: Runtime>(
// this is how the EVM behaves.
ContractType::Account | ContractType::NotFound => Ok(None),
// If we're calling a "native" actor, always revert.
ContractType::Native(_) => {
ContractType::Native => {
Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably keep the code CID and actually log it here.

IncorrectInputSize,
// FVM precompile errors
InvalidInput,
CallForbidden,
TransferFailed,
VMError(ActorError),
VMError,
}

impl From<ActorError> for PrecompileError {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to keep the error internals. I assume the complaint is that they're unused? In that case, we probably need to log them somewhere (will only affect actor debugging but it's still useful).

@rvagg
Copy link
Member

rvagg commented Dec 5, 2024

@Stebalien I've undone the dead code removals I originally pushed, one was easy to log, the other two I don't see an obvious place to log the output without either writing a new test just for that or spending more time that I have right now to figure out which tests might touch them and logging the specifics in there. Can you see a quick path to doing that, or should we just force override the linter on them?

@Stebalien
Copy link
Member Author

Clippy is a filthy liar. And it even tells us it's lying.

= note: `PrecompileError` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis

We log here:

log::warn!(target: "evm", "Precompile failed: error {:?}", err);

This gets rid of the clippy warning, and will make actor debug logs a
bit more readable.
@Stebalien
Copy link
Member Author

I've improved this a bit. I adds a tiny bit of code bloat, but it shouldn't be noticeable.

@rvagg
Copy link
Member

rvagg commented Dec 9, 2024

k, all lgtm, but maybe we should wait until after v16-final before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✔️ Approved by reviewer
Development

Successfully merging this pull request may close these issues.

2 participants