-
Notifications
You must be signed in to change notification settings - Fork 286
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
WIP: [Core] account abstraction (EIP-2938 ) integration #3785
Conversation
Merged changes from https://github.com/quilt/go-ethereumwith appropriate modifications to adapt for harmony evm and transaction pool.
@@ -908,6 +918,25 @@ func makeLog(size int) executionFunc { | |||
} | |||
} | |||
|
|||
func opPaygas(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memory *Memory, stack *Stack) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the reference implementation, they used callContext, why we don't have it here? https://github.com/quilt/go-ethereum/blob/9c4f387bc356af0e75019764d5730e5ea8b36d7f/core/vm/instructions.go#L914
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because call context seems to be introduced in more recent version ofthe vm than harmony uses currently. but symantically we do exactly the same: call context is a struct with stack, contract and memory, and we pass exactly the same values just as separate arguments.
@@ -242,7 +246,7 @@ func (fs FrontierSigner) SignatureValues(tx InternalTransaction, sig []byte) (r, | |||
func (fs FrontierSigner) Hash(tx InternalTransaction) common.Hash { | |||
return hash.FromRLP([]interface{}{ | |||
tx.Nonce(), | |||
tx.GasPrice(), | |||
tx.RawGasPrice(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason to change GasPrice to RawGasPrice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! as far as I remember, some harmony tests were failing because the new GasPrice()
implementation calls IsAA()
which assumes that transaction is signed, whereas it was not the case in tests. This should be symantically equivalent to what upstream implementation is doing (since they are not calling GasPrice()
, they do tx.data.Price
which equals to RawGasPrice()
).
hash atomic.Value | ||
size atomic.Value | ||
from atomic.Value | ||
hash atomic.Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually in harmony, EthTransaction is exactly the ethereum transaction format. So the code here should be very similar to the reference implementation in /core/types/transaction.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, and so it is. I tried to migrate&merge all relevant changes from upstream core/types/transaction.go. If you think there is some code missing here, please elaborate.
Review: add AccountAbstractionEpoch into config
thank you for your work. can you fix conflict? thanks. |
@@ -29,6 +29,11 @@ import ( | |||
staking "github.com/harmony-one/harmony/staking/types" | |||
) | |||
|
|||
type headerGetter interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why refactor the ChainContext into headerGetter? if it's not necessary, better keep as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because NewEVMContext()
is used in tx pool's validateTx()
and since pool's blockChain
attribute does not support the ChainContext interface. this way is the least changes to wire it together.
accomodate latest changes for PriceLimit, aa tests expect it to be 1, not 1e9 fix formatting to make goimports happy
@@ -210,28 +217,34 @@ func (st *StateTransition) to() common.Address { | |||
|
|||
func (st *StateTransition) useGas(amount uint64) error { | |||
if st.gas < amount { | |||
return vm.ErrOutOfGas | |||
return ErrIntrinsicGas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change this error to ErrIntrinsicGas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because TransitionDb()
in the reference implementation returns this error and it is expected in tests. from what I see, harmony implementation refactored the TransitionDb()
a little bit, moving out some stuff to other methods like useGas()
. thus I had to change useGas()
to be consistent with the reference implementation. In general, the intrinsic gas for a transaction is the amount of gas that the transaction uses before any code runs.
@@ -216,160 +251,183 @@ func newFrontierInstructionSet() JumpTable { | |||
maxStack: maxStack(0, 0), | |||
halts: true, | |||
valid: true, | |||
internal: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know why this new field internal are all true? if it's all true, what's the point of having this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see ~10 instructions that are not market as internal=true, for example: EXTCODEHASH, CREATE2, BALANCE, EXTCODESIZE, EXTCODECOPY, BLOCKHASH, COINBASE, ... Those are not allowed to be used in the AA transaction code before PAYGAS is executed.
I've went through all the codes and put my comments. I think the code in this PR are mostly looking good. You just need to test it with all the available tests to show that it's working as intended. You also need to show us a few examples by running the blockchain and sending AA txns showing that the txns are actually processed correctly and included in the block. |
A new pull request will be submitted for account abstraction pending further review from the protocol engineering team. Closing. |
Merged changes from https://github.com/quilt/go-ethereumwith appropriate modifications to adapt for harmony evm and transaction pool.
Currently most tests are passing that I can test locally. I haven't managed to run the official test suite from https://github.com/quilt/tests/tree/account-abstraction yet.
Since I am very new to harmony core internals I might be doing something completely wrong. So I welcome early feedback on the approach. Guidance about how to integrate with the official test suite is also most welcome.
Issue
harmony-one/bounties#35
Test
Unit Test Coverage
Before:
After:
Test/Run Logs
Operational Checklist
Does this PR introduce backward-incompatible changes to the on-disk data structure and/or the over-the-wire protocol?. (If no, skip to question 8.)
YES|NO
Describe the migration plan.. For each flag epoch, describe what changes take place at the flag epoch, the anticipated interactions between upgraded/non-upgraded nodes, and any special operational considerations for the migration.
Describe how the plan was tested.
How much minimum baking period after the last flag epoch should we allow on Pangaea before promotion onto mainnet?
What are the planned flag epoch numbers and their ETAs on Pangaea?
What are the planned flag epoch numbers and their ETAs on mainnet?
Note that this must be enough to cover baking period on Pangaea.
What should node operators know about this planned change?
Does this PR introduce backward-incompatible changes NOT related to on-disk data structure and/or over-the-wire protocol? (If no, continue to question 11.)
YES|NO
Does the existing
node.sh
continue to work with this change?What should node operators know about this change?
Does this PR introduce significant changes to the operational requirements of the node software, such as >20% increase in CPU, memory, and/or disk usage?
TODO