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

[stable2503] Backport pallet-revive fixes #7776

Merged
merged 6 commits into from
Mar 3, 2025
Merged

Conversation

athei
Copy link
Member

@athei athei commented Mar 3, 2025

Fixes #7705

pgherveou and others added 6 commits March 3, 2025 12:18
Various pallet-revive improvements

- add check for precompiles addresses,
So we can easily identify which one are being called and not supported
yet

- fixes debug_call for revert call
If a call revert we still want to get the traces for that call, that
matches geth behaviors, diff tests will be added to the test suite for
this

- fixes traces for staticcall
The call type was not always being reported properly.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <[email protected]>
Fix tracing should wrap around the entire call stack execution

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Add ECrecover 0x1 precompile and remove the unstable equivalent host
function.

- depend on #7676

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <[email protected]>
This PR changes the behavior of delegate calls when the callee is not a
contract account: Instead of returning a `CodeNotFound` error, this is
allowed and the caller observes a successful call with empty output.

The change makes for example the following contract behave the same as
on EVM:

```Solidity
contract DelegateCall {
    function delegateToLibrary() external returns (bool) {
        address testAddress = 0x0000000000000000000000000000000000000000;
        (bool success, ) = testAddress.delegatecall(
            abi.encodeWithSignature("test()")
        );
        return success;
    }
}
```

Closes paritytech/revive#235

---------

Signed-off-by: xermicus <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #6723

Internal auditors recommended to not truncate Polkadot Addresses when
deriving Ethereum addresses from it. Reasoning is that they are raw
public keys where truncating could lead to collisions when weaknesses in
those curves are discovered in the future. Additionally, some pallets
generate account addresses in a way where only the suffix we were
truncating contains any entropy. The changes in this PR act as a safe
guard against those two points.

We change the `to_address` function to first hash the AccountId32 and
then use trailing 20 bytes as `AccountId20`. If the `AccountId32` ends
with 12x 0xEE we keep our current behaviour of just truncating those
trailing bytes.

This will allow us to still recover the original `AccountId20` because
those are constructed by just adding those 12 bytes. Please note that
generating an ed25519 key pair where the trailing 12 bytes are 0xEE is
theoretically possible as 96bits is not a huge search space. However,
this cannot be used as an attack vector. It will merely allow this
address to interact with `pallet_revive` without registering as the
fallback account is the same as the actual address. The ultimate vanity
address. In practice, this is not relevant since the 0xEE addresses are
not valid public keys for sr25519 which is used almost everywhere.

tl:dr: We keep truncating in case of an Ethereum address derived account
id. This is safe as those are already derived via keccak. In every other
case where we have to assume that the account id might be a public key.
Therefore we first hash and then take the trailing bytes.

No. We changed the name of the mapping. This means the runtime will not
try to read the old data. Ethereum keys are unaffected by this change.
We just advise people to re-register their AccountId32 in case they need
to use it as it is a very small circle of users (just 3 addresses
registered). This will not cause disturbance on Westend.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #6157

This fixes the last remaining benchmark that was not correct since it
was too low level to be written in Rust. Instead, we opted.

This PR changes the benchmark that determines the scaling from
`ref_time` to PolkaVM `Gas` by benchmarking the absolute worst case of
an instruction: One that causes two cache misses by touching two cache
lines.

The Contract itself is designed to be as simple as possible. It does
random unaligned reads in a loop until the `r` (repetition) number is
reached. The randomness is fully generated by the host and written to
the guests memory before the benchmark is run. This allows the benchmark
to determine the influence of one loop iteration via linear regression.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: xermicus <[email protected]>
Co-authored-by: PG Herveou <[email protected]>
@athei athei added the R0-silent Changes should not be mentioned in any release notes label Mar 3, 2025
Copy link
Contributor

github-actions bot commented Mar 3, 2025

This pull request is amending an existing release. Please proceed with extreme caution,
as to not impact downstream teams that rely on the stability of it. Some things to consider:

  • Backports are only for 'patch' or 'minor' changes. No 'major' or other breaking change.
  • Should be a legit fix for some bug, not adding tons of new features.
  • Must either be already audited or not need an audit.
Emergency Bypass

If you really need to bypass this check: add validate: false to each crate
in the Prdoc where a breaking change is introduced. This will release a new major
version of that crate and all its reverse dependencies and basically break the release.

@athei
Copy link
Member Author

athei commented Mar 3, 2025

cargo-check-benches (master) fails because on master it requires a newer Rust which is apparently not set on this branch. So I suggest we ignore that. The fix would be that if you compile master you should also use the CI image that is declared by master.

@EgorPopelyaev EgorPopelyaev merged commit 773ffdf into stable2503 Mar 3, 2025
243 of 260 checks passed
@EgorPopelyaev EgorPopelyaev deleted the at/backport branch March 3, 2025 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants