-
Notifications
You must be signed in to change notification settings - Fork 773
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
Implement EIP2718 and EIP2930 #1048
Changes from all commits
3fabbb3
76af18f
1847d8b
5f2a1c9
f9852ca
7a35c75
64cb4c7
946e35a
ac694fb
2943360
2b78645
73f4b35
25ec7d5
ae533d2
c5c58d0
cd6a58f
bdc0d3b
0b8643d
87955c8
a638d8b
cc928ee
2385c6f
cca3b88
af6782f
3539369
4a7a547
9ececec
ceeedc6
7d12beb
861d1c0
be27b71
9c6f2f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
[submodule "ethereum-tests"] | ||
path = packages/ethereum-tests | ||
url = https://github.com/ethereum/tests.git | ||
url = https://github.com/qbzzt/tests.git | ||
branch = develop |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{ | ||
"name": "EIP-2718", | ||
"comment": "Typed Transaction Envelope", | ||
"url": "https://eips.ethereum.org/EIPS/eip-2718", | ||
"status": "Draft", | ||
"minimumHardfork": "chainstart", | ||
"gasConfig": {}, | ||
"gasPrices": {}, | ||
"vm": {}, | ||
"pow": {} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
{ | ||
"name": "EIP-2929", | ||
"comment": "Optional access lists", | ||
"url": "https://eips.ethereum.org/EIPS/eip-2930", | ||
"status": "Draft", | ||
"minimumHardfork": "istanbul", | ||
"requiredEIPs": [2718, 2929], | ||
"gasConfig": {}, | ||
"gasPrices": { | ||
"accessListStorageKeyCost": { | ||
"v": 1900, | ||
"d": "Gas cost per storage key in an Access List transaction" | ||
}, | ||
"accessListAddressCost": { | ||
"v": 2400, | ||
"d": "Gas cost per storage key in an Access List transaction" | ||
} | ||
}, | ||
"vm": {}, | ||
"pow": {} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,195 @@ | ||
import Common from '@ethereumjs/common' | ||
import { | ||
Address, | ||
BN, | ||
toBuffer, | ||
MAX_INTEGER, | ||
unpadBuffer, | ||
ecsign, | ||
publicToAddress, | ||
} from 'ethereumjs-util' | ||
import { BaseTransactionData, BaseTxOptions, DEFAULT_COMMON, JsonTx } from './types' | ||
|
||
export abstract class BaseTransaction<TransactionObject> { | ||
public readonly nonce: BN | ||
public readonly gasLimit: BN | ||
public readonly gasPrice: BN | ||
public readonly to?: Address | ||
public readonly value: BN | ||
public readonly data: Buffer | ||
public readonly common: Common | ||
|
||
constructor(txData: BaseTransactionData, txOptions: BaseTxOptions = {}) { | ||
const { nonce, gasLimit, gasPrice, to, value, data } = txData | ||
|
||
this.nonce = new BN(toBuffer(nonce)) | ||
this.gasPrice = new BN(toBuffer(gasPrice)) | ||
this.gasLimit = new BN(toBuffer(gasLimit)) | ||
this.to = to ? new Address(toBuffer(to)) : undefined | ||
this.value = new BN(toBuffer(value)) | ||
this.data = toBuffer(data) | ||
|
||
const validateCannotExceedMaxInteger = { | ||
nonce: this.nonce, | ||
gasPrice: this.gasPrice, | ||
gasLimit: this.gasLimit, | ||
value: this.value, | ||
} | ||
|
||
this.validateExceedsMaxInteger(validateCannotExceedMaxInteger) | ||
|
||
this.common = | ||
(txOptions.common && | ||
Object.assign(Object.create(Object.getPrototypeOf(txOptions.common)), txOptions.common)) ?? | ||
DEFAULT_COMMON | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, likely best to directly set to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we set default common to Berlin, then these EIPs are activated by default right? Also a side note: I think we should move There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
that's a great idea There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have been thinking about this a bit, this brings again up the compatibility question. So I guess we can't set to Might be a bit counterintuitive on first thought, but I guess the technically clean way to do it is to keep default on plain Together with the WDYGT? 🤔
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I agree here - I don't think it makes sense to default it to Berlin, especially since it is not yet activated on mainnet. It would be very weird to default the VM to currently non-activated new logic. About creating a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue #1128 Again, it would make sense that we default to Istanbul. It is therefore not possible to create |
||
} | ||
|
||
protected validateExceedsMaxInteger(validateCannotExceedMaxInteger: { [key: string]: BN }) { | ||
for (const [key, value] of Object.entries(validateCannotExceedMaxInteger)) { | ||
if (value && value.gt(MAX_INTEGER)) { | ||
throw new Error(`${key} cannot exceed MAX_INTEGER, given ${value}`) | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* If the tx's `to` is to the creation address | ||
*/ | ||
toCreationAddress(): boolean { | ||
return this.to === undefined || this.to.buf.length === 0 | ||
} | ||
|
||
/** | ||
* Computes a sha3-256 hash of the serialized unsigned tx, which is used to sign the transaction. | ||
*/ | ||
rawTxHash(): Buffer { | ||
return this.getMessageToSign() | ||
} | ||
|
||
abstract getMessageToSign(): Buffer | ||
|
||
/** | ||
* Returns chain ID | ||
*/ | ||
getChainId(): number { | ||
return this.common.chainId() | ||
} | ||
|
||
/** | ||
* The amount of gas paid for the data in this tx | ||
*/ | ||
getDataFee(): BN { | ||
const txDataZero = this.common.param('gasPrices', 'txDataZero') | ||
const txDataNonZero = this.common.param('gasPrices', 'txDataNonZero') | ||
|
||
let cost = 0 | ||
for (let i = 0; i < this.data.length; i++) { | ||
this.data[i] === 0 ? (cost += txDataZero) : (cost += txDataNonZero) | ||
} | ||
return new BN(cost) | ||
} | ||
|
||
/** | ||
* The minimum amount of gas the tx must have (DataFee + TxFee + Creation Fee) | ||
*/ | ||
getBaseFee(): BN { | ||
const fee = this.getDataFee().addn(this.common.param('gasPrices', 'tx')) | ||
if (this.common.gteHardfork('homestead') && this.toCreationAddress()) { | ||
fee.iaddn(this.common.param('gasPrices', 'txCreation')) | ||
} | ||
return fee | ||
} | ||
|
||
/** | ||
* The up front amount that an account must have for this transaction to be valid | ||
*/ | ||
getUpfrontCost(): BN { | ||
return this.gasLimit.mul(this.gasPrice).add(this.value) | ||
} | ||
|
||
/** | ||
* Checks if the transaction has the minimum amount of gas required | ||
* (DataFee + TxFee + Creation Fee). | ||
*/ | ||
/** | ||
* Checks if the transaction has the minimum amount of gas required | ||
* (DataFee + TxFee + Creation Fee). | ||
*/ | ||
validate(): boolean | ||
/* eslint-disable-next-line no-dupe-class-members */ | ||
jochem-brouwer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
validate(stringError: false): boolean | ||
/* eslint-disable-next-line no-dupe-class-members */ | ||
validate(stringError: true): string[] | ||
/* eslint-disable-next-line no-dupe-class-members */ | ||
validate(stringError: boolean = false): boolean | string[] { | ||
const errors = [] | ||
|
||
if (this.getBaseFee().gt(this.gasLimit)) { | ||
errors.push(`gasLimit is too low. given ${this.gasLimit}, need at least ${this.getBaseFee()}`) | ||
} | ||
|
||
if (this.isSigned() && !this.verifySignature()) { | ||
errors.push('Invalid Signature') | ||
} | ||
|
||
return stringError ? errors : errors.length === 0 | ||
} | ||
|
||
/** | ||
* Returns the encoding of the transaction. | ||
*/ | ||
abstract serialize(): Buffer | ||
|
||
/** | ||
* Returns an object with the JSON representation of the transaction | ||
*/ | ||
abstract toJSON(): JsonTx | ||
|
||
abstract isSigned(): boolean | ||
|
||
/** | ||
* Determines if the signature is valid | ||
*/ | ||
verifySignature(): boolean { | ||
try { | ||
// Main signature verification is done in `getSenderPublicKey()` | ||
const publicKey = this.getSenderPublicKey() | ||
return unpadBuffer(publicKey).length !== 0 | ||
} catch (e) { | ||
return false | ||
} | ||
} | ||
|
||
/** | ||
* Returns the raw `Buffer[]` (LegacyTransaction) or `Buffer` (typed transaction). | ||
* This is the data which is found in the transactions of the block body. | ||
*/ | ||
abstract raw(): Buffer[] | Buffer | ||
abstract hash(): Buffer | ||
|
||
abstract getMessageToVerifySignature(): Buffer | ||
/** | ||
* Returns the sender's address | ||
*/ | ||
getSenderAddress(): Address { | ||
return new Address(publicToAddress(this.getSenderPublicKey())) | ||
} | ||
abstract getSenderPublicKey(): Buffer | ||
|
||
sign(privateKey: Buffer): TransactionObject { | ||
if (privateKey.length !== 32) { | ||
throw new Error('Private key must be 32 bytes in length.') | ||
} | ||
|
||
const msgHash = this.getMessageToSign() | ||
|
||
// Only `v` is reassigned. | ||
/* eslint-disable-next-line prefer-const */ | ||
let { v, r, s } = ecsign(msgHash, privateKey) | ||
|
||
return this.processSignature(v, r, s) | ||
} | ||
|
||
// Accept the v,r,s values from the `sign` method, and convert this into a TransactionObject | ||
protected abstract processSignature(v: number, r: Buffer, s: Buffer): TransactionObject | ||
} |
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.
Was thinking a bit about this breaking part in
Block
.So if I get this right the only breaking thing here would be
Transaction
now being a type in the main constructor, right?Wouldn't we already back on the compatibility track if we rename this new type
Transaction
->TypedTransaction
(not sure, or another name, I would think though that this name is justified even if it encompassed the old transaction format, to be discussed) and thenLegacyTransaction
back toTransaction
?I actually like this in general (we might still want to rename back
Transaction
->LegacyTransaction
on a general breaking-release-round on all libraries, not sure), since having the type namedTypedTransaction
more clearly indicates that a function now allows for a typed tx as an input.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.
We can do this, but this still implies we have to use the
TransactionFactory
inBlock
, and we also have to use theTypedTransactions
as the type for thetransactions
field in the block body. We can indeed export theLegacyTransaction
as aTransaction
, this would mean a non-breaking release forTransaction
, but due to the type changes onBlock
they are breaking. (I am not entirely sure if they are completely breaking, since all methods of the oldTransaction
are available on bothEIP2930Transaction
andLegacyTransaction
, so maybe it is not strictly breaking?)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.
The
TransactionFactory
inBlock
is not a problem. With a newBlock
release theTransaction
dependency is also bumped, so the newBlock
version for sure works together with the newTransaction
version release. So that is no problem.Compatibility problems only arise on direct API usage, there, the only thing we have is the constructor:
This is the only part which we need to make sure that it still takes in
Transaction
objects from the current library version, which I think it should when we have a typeEIP2930Transaction | Transaction
(we should test this though, this would be an application for @cgewecke using the functionality from #1118.If there is the possibility we should very much do this. This is so so crucial that we remain backwards-compatible (especially if it's just a case about a simple rename).
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 will rebase #1118 against this and open as draft.
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.
Cool! 😄 Can you do the suggested renaming (see above) also along for test purposes? So this would be needed to see if this will work.
Any opinion if we now might want to merge this in here? I would be still a fan, think this would ease follow-up development. Doesn't mean that we take this out of review. We can just do follow-up fixes on future PRs.