[EVM] associate via bytes conversion #1706

stevenlanders · 2024-05-29T01:40:27Z

Describe your changes and provide context

Going forward, we will stop supporting address association based on pubkey derivation, but use direct casting for all accounts who haven't already established pubkey-based association. Existing pubkey-based association will still be respected. Namely the following changes are made:

the address lookup functions will resort to direct casting if no association is found in db
AssociateTx type is removed
No pubkey-based association will be established for either EVM txs or Cosmos txs
No association will be set for new EVM code
Legacy association logic is still supported in trace endpoints

Testing performed to validate your change

unit tests

x/evm/keeper/address.go

codecov · 2024-05-30T06:43:28Z

Codecov Report

Attention: Patch coverage is 76.66667% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 60.90%. Comparing base (1702e4e) to head (cbef8fc).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1706      +/-   ##
==========================================
+ Coverage   60.69%   60.90%   +0.20%     
==========================================
  Files         370      368       -2     
  Lines       27279    26930     -349     
==========================================
- Hits        16558    16402     -156     
+ Misses       9585     9433     -152     
+ Partials     1136     1095      -41

Files	Coverage Δ
aclmapping/evm/mappings.go	`92.17% <100.00%> (+3.09%)`	⬆️
app/antedecorators/gasless.go	`83.45% <ø> (+5.28%)`	⬆️
evmrpc/association.go	`58.33% <100.00%> (+27.49%)`	⬆️
evmrpc/server.go	`86.61% <100.00%> (ø)`
precompiles/bank/bank.go	`58.09% <100.00%> (+1.41%)`	⬆️
precompiles/distribution/distribution.go	`52.47% <100.00%> (+3.82%)`	⬆️
precompiles/gov/gov.go	`55.78% <100.00%> (+3.31%)`	⬆️
precompiles/staking/staking.go	`67.69% <100.00%> (+4.38%)`	⬆️
precompiles/wasmd/wasmd.go	`63.80% <100.00%> (+1.93%)`	⬆️
x/evm/ante/error.go	`100.00% <ø> (ø)`
... and 21 more

... and 3 files with indirect coverage changes

udpatil · 2024-05-30T13:55:27Z

aclmapping/evm/mappings.go


 	if err := ante.Preprocess(ctx, evmMsg); err != nil {
 		return []sdkacltypes.AccessOperation{}, err
 	}
 	ops := []sdkacltypes.AccessOperation{}
 	ops = appendRWBalanceOps(ops, state.GetCoinbaseAddress(ctx.TxIndex()))
-	sender := evmMsg.Derived.SenderSeiAddr
+	sender := evmKeeper.GetSeiAddress(ctx, evmMsg.Derived.SenderEVMAddr)


we should be able to remove this ACLMapping behavior given that parallelv1 is no longer used, its either OCC or synchronous

would these still be used by OCC to guesstimate potential conflicts?

we've disabled that since it didn't show any realistic gains in high conflict scenarios and was only adding latency to happy paths:
https://github.com/sei-protocol/sei-cosmos/blob/8dd4adcf7553c056ee338dabb155bdf03821a99d/tasks/scheduler.go#L291

udpatil · 2024-05-30T13:58:18Z

docker/localnode/scripts/step2_genesis.sh

@@ -46,6 +46,9 @@ override_genesis '.app_state["gov"]["tally_params"]["threshold"]="0.5"'
 override_genesis '.app_state["gov"]["tally_params"]["expedited_quorum"]="0.9"'
 override_genesis '.app_state["gov"]["tally_params"]["expedited_threshold"]="0.9"'

+# evm config
+override_genesis '.app_state["evm"]["address_associations"]=[{"sei_address":"sei1m9qugvk4h66p6hunfajfg96ysc48zeq4m0d82c","eth_address":"0xF87A299e6bC7bEba58dbBe5a5Aa21d49bCD16D52"},{"sei_address":"sei1cjzphr67dug28rw9ueewrqllmxlqe5f0awulvy","eth_address":"0x70997970C51812dc3A010C7d01b50e0d17dc79C8"}]'


are these older style of associated addresses for testing coverage?

udpatil · 2024-05-30T14:05:01Z

x/evm/client/cli/native_tx.go

@@ -56,6 +58,54 @@ func NativeSendTxCmd() *cobra.Command {
 	return cmd
 }

+func SendCastSeiAddrTxCmd() *cobra.Command {


this should basically be a no-op right? as in, is this intended solely for testing a self-send? if so how is this different from doing seid tx evm send <cast-addr> <amount> --from admin?

say admin's true sei address is seiABC and true evm address is 0xXYZ: seid tx evm send <cast-addr> <amount> --from admin would be an EVM tx with an EVM sig that decodes to the true evm address 0xXYZ. The direct cast address of it sei(0xXYZ) doesn't have any fund to send because funds are under seiABC. This new command is a cosmos tx that sends from seiABC to sei(0xXYZ)

understood, ty for exaplanation

udpatil · 2024-05-30T16:21:53Z

evmrpc/simulate.go

+		addressThen := b.keeper.GetSeiAddress(statedb.Ctx(), msg.From)
+		addressNow := b.keeper.GetSeiAddress(b.ctxProvider(LatestCtxHeight), msg.From)
+		if !addressThen.Equals(addressNow) {
+			if err := ante.NewEVMPreprocessDecorator(b.keeper, b.keeper.AccountKeeper()).AssociateAddresses(statedb.Ctx(), addressNow, msg.From, nil); err != nil {


why are we associating the address with addressNow since now any association is implicit, right? I dont know that I fully understand this edge case, is it where you want to simulate a tx on an old block height prior to this change so previously it would have registered the old style associaton, but because NOW the logic is different, we associate the new correct address instead? If so, I still dont understand why we need to explicitly associate as opposed use the implicit casting?

this is for legacy association created before this change. addressThen is the sei address before THAT legacy association happened. What we are doing here is basically checking if this tx, back in time, caused a legacy association, and reproduce that if so.

I think the var name might be what's confusing: addressNow doesn't mean the current way of address association, rather it means the association of the address in the latest DB, and it could be a legacy one

ahh gotcha, yeah the addressNow name was confusing me

udpatil · 2024-05-30T16:47:29Z

evmrpc/simulate.go

+		addressThen := b.keeper.GetSeiAddress(statedb.Ctx(), msg.From)
+		addressNow := b.keeper.GetSeiAddress(b.ctxProvider(LatestCtxHeight), msg.From)
+		if !addressThen.Equals(addressNow) {
+			if err := ante.NewEVMPreprocessDecorator(b.keeper, b.keeper.AccountKeeper()).AssociateAddresses(statedb.Ctx(), addressNow, msg.From, nil); err != nil {


I don't fully understand this logic, is the intention to allow simulating txs on older blocks where a tx would have associated the old way? I'm not fully understanding the relationship here between oldAddress and newAddress and how its needed here.

see above. An example would be:

tx T created a legacy association for account 0xXYZ at height 100

latest height is 200

when we trace T, we start with the state at height 99

addressThen here would be the association of 0xXYZ at height 99, which is the direct cast because T hasn't materialized yet

addressNow here would be the association at height 200, which would be the legacy association as it's set on height 100

since those two are different, we know an address association is set on height 100 (and only a legacy association can cause the difference), so we simulate it here

ty for explanation

udpatil · 2024-05-30T19:34:57Z

x/evm/keeper/address.go

-	addr, ok := k.GetSeiAddress(ctx, evmAddress)
-	if ok {
-		return addr
+		return evmAddress.Bytes()


do we not need to do something like sdk.AccAddress(evmAddress.Bytes())?

it's cast implicitly

udpatil · 2024-05-30T19:35:25Z

x/evm/keeper/address.go

 	}
-	return sdk.AccAddress(evmAddress[:])
+	return bz


codchen reviewed May 29, 2024

View reviewed changes

x/evm/keeper/address.go Outdated Show resolved Hide resolved

codchen added the app-hash-breaking label May 30, 2024

codchen marked this pull request as ready for review May 30, 2024 06:09

codchen force-pushed the associate-v2 branch from 987421d to 19738bf Compare May 30, 2024 07:20

stevenlanders changed the title ~~[POC] associate via bytes conversion~~ [EVM] associate via bytes conversion May 30, 2024

udpatil reviewed May 30, 2024

View reviewed changes

udpatil self-requested a review May 30, 2024 19:57

codchen force-pushed the associate-v2 branch 4 times, most recently from 11da3de to 70b1cfd Compare May 31, 2024 06:05

stevenlanders force-pushed the associate-v2 branch from 70b1cfd to 1178979 Compare May 31, 2024 13:27

stevenlanders and others added 7 commits June 3, 2024 09:16

associate via bytes conversion

3b61ebc

deprecate associateTx

3ee8dd7

fix tests

3b0812d

integration tests

38fc863

adjust default timeouts for integration tests

64d4293

set timeout to 500s

8c633ff

fix tests

cbef8fc

stevenlanders force-pushed the associate-v2 branch from 1178979 to cbef8fc Compare June 3, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EVM] associate via bytes conversion #1706

[EVM] associate via bytes conversion #1706

stevenlanders commented May 29, 2024 •

edited by codchen

Loading

codecov bot commented May 30, 2024 •

edited

Loading

udpatil May 30, 2024

codchen May 31, 2024

udpatil May 31, 2024

udpatil May 30, 2024

codchen May 31, 2024

udpatil May 30, 2024

codchen May 31, 2024

udpatil May 31, 2024

udpatil May 30, 2024

codchen May 31, 2024

udpatil May 31, 2024

udpatil May 30, 2024

codchen May 31, 2024

udpatil May 31, 2024

udpatil May 30, 2024

codchen May 31, 2024

udpatil May 30, 2024

[EVM] associate via bytes conversion #1706

Are you sure you want to change the base?

[EVM] associate via bytes conversion #1706

Conversation

stevenlanders commented May 29, 2024 • edited by codchen Loading

Describe your changes and provide context

Testing performed to validate your change

codecov bot commented May 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenlanders commented May 29, 2024 •

edited by codchen

Loading

codecov bot commented May 30, 2024 •

edited

Loading