-
Notifications
You must be signed in to change notification settings - Fork 771
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
Refactor tx lib #812
Refactor tx lib #812
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
This comment has been minimized.
This comment has been minimized.
Love this. |
0f6cf99
to
c1a5d06
Compare
@alcuadrado would you like me to take this one step further with your implementation example in ethereumjs/organization#56? there are some additional ideas there not implemented in this PR yet:
|
Great PR!
I think this would be great. It would mean removing The freezing one may need more discussion. But if we are going to move to a more immutable API, I think TX is the best candidate to start with. Also, not that freezing is not actually needed for that, but it's a good way of self imposing it. |
@s1na would you have any idea why this test is now failing with "revert" instead of "value overflow"? the test above it seems fine, so it's not immediately obvious to me, and i didn't see any "revert reason" on the exception or error object to help point toward where it might have failed 🤔
|
@ryanio hm it doesn't immediately pop into my head why a |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This pull request fixes 1 alert when merging 5963fe2 into b276092 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging f8671fe into b276092 - view on LGTM.com fixed alerts:
|
Did a little bit of debugging here on the failing blockchain tests. When concentrating on running a single file with failing tests - e.g. with try {
result = await applyBlock.bind(this)(block, opts)
console.log(result)
} catch (err) {
console.log(err)
await state.revert()
throw err
} current $ ts-node ./tests/tester --blockchain --file='bcForkBlockTest'
+--------------------------------------------------+
| VM -> BlockchainTests |
| |
| TestGetterArgs |
| skipTests : 54 |
| forkConfig : Istanbul |
| file : bcForkBlockTest |
| |
| RunnerArgs |
| forkConfigVM : istanbul |
| forkConfigTestSuite : Istanbul |
+--------------------------------------------------+
TAP version 13
# BlockchainTests
# file: bcForkBlockTest test: BlockExtraData25_Istanbul
ok 1 correct pre stateRoot
ok 2 correct genesis RLP
Error: invalid amount of extra data
at BlockHeader.<anonymous> (/ethereumjs-vm/packages/block/src/header.ts:320:13)
at step (/ethereumjs-vm/packages/block/dist/header.js:33:23)
at Object.next (/ethereumjs-vm/packages/block/dist/header.js:14:53)
at fulfilled (/ethereumjs-vm/packages/block/dist/header.js:5:58)
ok 3 Expected exception ExtraDataTooBig
# file: bcForkBlockTest test: BlockWrongResetGas_Istanbul
ok 4 correct pre stateRoot
ok 5 correct genesis RLP
Error: Invalid Signature at tx 0
at Block.<anonymous> (/ethereumjs-vm/packages/block/src/block.ts:175:13)
at step (/ethereumjs-vm/packages/block/dist/block.js:33:23)
at Object.next (/ethereumjs-vm/packages/block/dist/block.js:14:53)
at fulfilled (/ethereumjs-vm/packages/block/dist/block.js:5:58)
at runNextTicks (internal/process/task_queues.js:59:5)
at processImmediate (internal/timers.js:412:9)
at process.topLevelDomainCallback (domain.js:130:23)
ok 6 Expected exception InvalidTransactionVRS
# file: bcForkBlockTest test: BlockWrongStoreClears_Istanbul
ok 7 correct pre stateRoot
ok 8 correct genesis RLP
{
bloom: Bloom {
bitvector: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 206 more bytes>
},
gasUsed: <BN: 59e7>,
receiptRoot: <Buffer 6c 8a b3 6e c0 62 9c 97 73 4e 8a c8 23 cd d8 39 7d e6 7e fb 76 c7 be b9 83 be 73 dc d3 c7 81 41>,
receipts: [
{
status: 1,
gasUsed: <Buffer 59 e7>,
bitvector: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 206 more bytes>,
logs: []
}
],
results: [
{
gasUsed: <BN: 59e7>,
execResult: [Object],
bloom: [Bloom],
amountSpent: <BN: 38306>
}
]
}
ok 9 Expected exception InvalidGasUsed
# file: bcForkBlockTest test: BlockWrongStoreSetGas_Istanbul
ok 10 correct pre stateRoot
ok 11 correct genesis RLP
Error: Invalid Signature at tx 0
at Block.<anonymous> (/ethereumjs-vm/packages/block/src/block.ts:175:13)
at step (/ethereumjs-vm/packages/block/dist/block.js:33:23)
at Object.next (/ethereumjs-vm/packages/block/dist/block.js:14:53)
at fulfilled (/ethereumjs-vm/packages/block/dist/block.js:5:58)
at runNextTicks (internal/process/task_queues.js:59:5)
at processImmediate (internal/timers.js:412:9)
at process.topLevelDomainCallback (domain.js:130:23)
ok 12 Expected exception InvalidTransactionVRS
# file: bcForkBlockTest test: SimpleTxCosts20000_Istanbul
ok 13 correct pre stateRoot
ok 14 correct genesis RLP
Error: Invalid Signature at tx 0
at Block.<anonymous> (/ethereumjs-vm/packages/block/src/block.ts:175:13)
at step (/ethereumjs-vm/packages/block/dist/block.js:33:23)
at Object.next (/ethereumjs-vm/packages/block/dist/block.js:14:53)
at fulfilled (/ethereumjs-vm/packages/block/dist/block.js:5:58)
at runNextTicks (internal/process/task_queues.js:59:5)
at processImmediate (internal/timers.js:412:9)
at process.topLevelDomainCallback (domain.js:130:23)
ok 15 Expected exception InvalidTransactionVRS
1..15
# tests 15
# pass 15 While running the same on the $ ts-node ./tests/tester --blockchain --file='bcForkBlockTest'
+--------------------------------------------------+
| VM -> BlockchainTests |
| |
| TestGetterArgs |
| skipTests : 54 |
| forkConfig : Istanbul |
| file : bcForkBlockTest |
| |
| RunnerArgs |
| forkConfigVM : istanbul |
| forkConfigTestSuite : Istanbul |
+--------------------------------------------------+
TAP version 13
# BlockchainTests
# file: bcForkBlockTest test: BlockExtraData25_Istanbul
ok 1 correct pre stateRoot
ok 2 correct genesis RLP
ok 3 Expected exception ExtraDataTooBig
# file: bcForkBlockTest test: BlockWrongResetGas_Istanbul
ok 4 correct pre stateRoot
ok 5 correct genesis RLP
{
bloom: Bloom {
bitvector: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 206 more bytes>
},
gasUsed: <BN: 6596>,
receiptRoot: <Buffer ab 6d 3e 30 83 dd ef b7 cd 46 3e 12 70 c0 7a 12 6b 51 ab 73 6a 8d 8d cc 95 a9 10 a7 3e ce 71 e9>,
receipts: [
{
status: 1,
gasUsed: <Buffer 65 96>,
bitvector: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 206 more bytes>,
logs: []
}
],
results: [
{
gasUsed: <BN: 6596>,
execResult: [Object],
bloom: [Bloom],
amountSpent: <BN: 3f7dc>
}
]
}
not ok 6 last block hash
---
operator: equal
expected: |-
'8d3d0d122068250849fce79eb37f1e6838887d298d443e5b40174333b83406ec'
actual: |-
'78aedd8a1a7eeba4823317a7141e37bf8c5d6369e239e82ab674c33286076f05'
at: runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:139:11)
stack: |-
Error: last block hash
at Test.assert [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:228:54)
at Test.bound [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at Test.equal (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:389:10)
at Test.bound (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:139:11)
at /ethereumjs-vm-tx-refactor/packages/vm/tests/tester.js:112:13
at fileCallback (/ethereumjs-vm-tx-refactor/packages/vm/tests/testLoader.js:56:11)
...
not ok 7 correct header block
---
operator: equal
expected: |-
'8d3d0d122068250849fce79eb37f1e6838887d298d443e5b40174333b83406ec'
actual: |-
'78aedd8a1a7eeba4823317a7141e37bf8c5d6369e239e82ab674c33286076f05'
at: runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:156:11)
stack: |-
Error: correct header block
at Test.assert [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:228:54)
at Test.bound [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at Test.equal (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:389:10)
at Test.bound (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:156:11)
at /ethereumjs-vm-tx-refactor/packages/vm/tests/tester.js:112:13
at fileCallback (/ethereumjs-vm-tx-refactor/packages/vm/tests/testLoader.js:56:11)
...
# file: bcForkBlockTest test: BlockWrongStoreClears_Istanbul
ok 8 correct pre stateRoot
ok 9 correct genesis RLP
{
bloom: Bloom {
bitvector: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 206 more bytes>
},
gasUsed: <BN: 59e7>,
receiptRoot: <Buffer 6c 8a b3 6e c0 62 9c 97 73 4e 8a c8 23 cd d8 39 7d e6 7e fb 76 c7 be b9 83 be 73 dc d3 c7 81 41>,
receipts: [
{
status: 1,
gasUsed: <Buffer 59 e7>,
bitvector: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 206 more bytes>,
logs: []
}
],
results: [
{
gasUsed: <BN: 59e7>,
execResult: [Object],
bloom: [Bloom],
amountSpent: <BN: 38306>
}
]
}
not ok 10 last block hash
---
operator: equal
expected: |-
'fe68e7d76854990d722d204f2eccc1a8c10a730deb7816cf28bbb7f3dbbcaacf'
actual: |-
'22f5ba1ed9ba7a09b9bc8d6f8c4a182c30e4219c775d47e2bd3f40ba5bcc260d'
at: runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:139:11)
stack: |-
Error: last block hash
at Test.assert [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:228:54)
at Test.bound [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at Test.equal (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:389:10)
at Test.bound (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:139:11)
at /ethereumjs-vm-tx-refactor/packages/vm/tests/tester.js:112:13
at fileCallback (/ethereumjs-vm-tx-refactor/packages/vm/tests/testLoader.js:56:11)
...
not ok 11 correct header block
---
operator: equal
expected: |-
'fe68e7d76854990d722d204f2eccc1a8c10a730deb7816cf28bbb7f3dbbcaacf'
actual: |-
'22f5ba1ed9ba7a09b9bc8d6f8c4a182c30e4219c775d47e2bd3f40ba5bcc260d'
at: runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:156:11)
stack: |-
Error: correct header block
at Test.assert [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:228:54)
at Test.bound [as _assert] (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at Test.equal (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:389:10)
at Test.bound (/ethereumjs-vm-tx-refactor/node_modules/tape/lib/test.js:80:32)
at runBlockchainTest (/ethereumjs-vm-tx-refactor/packages/vm/tests/BlockchainTestsRunner.js:156:11)
at /ethereumjs-vm-tx-refactor/packages/vm/tests/tester.js:112:13
at fileCallback (/ethereumjs-vm-tx-refactor/packages/vm/tests/testLoader.js:56:11)
...
// I'll cut here a bit due to getting too bulky
1..19
# tests 19
# pass 11
# fail 8 So when looking at the expected exceptions Just an on-first-sight interpretation, it might also be the case that the fixes here are correct and our test setup is wrong, this is less likely though I would say. The work from @jochem-brouwer on #849 should help to make such kind of analysis less disambiguous by running the tests to the end (these early stops on test runs are also the reason for the differing in tests executed on the comparison from above should - I would assume - be unrelated to the root of the problem though). |
@holgerd77 FYI that injection of code is semi-equivalent to hooking up to the Just glanced over these blockchain tests, very confusing, these have to do with block tests 🤔 |
public readonly r?: BN | ||
public readonly s?: BN | ||
|
||
public static fromTxData(txData: TxData, common?: Common) { |
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.
Just realizing while working on a similar transition on the block library: do we want to put common
here in an options dict? Think this would be more future-proof, will also introduce the like on block since there are options which needed to be passed around.
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.
hmm I liked the simplicity of removing the need for TxOptions, but i see your point for future-proofing.
i assume we would do it for the constructor and all the factory methods?
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.
Yes, would say so, please also put TxOptions being the last parameter for the constructor for consistency (unless you have got an explicit reason not to).
Yeah, sorry for un-simplify here a bit, but I think this is quite a realistic scenario that we want to add here at some point and it would be a bit a pity if this would be blocked by not have this prepared.
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.
LGTM
Some last comments for the next cosmetic finalization round. 😄
Will merge.
@@ -29,6 +29,7 @@ export default async function runBlockchain(this: VM, blockchain?: Blockchain) { | |||
// run block, update head if valid | |||
try { | |||
await this.runBlock({ block, root: parentState }) | |||
console.log() |
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.
Ug. 😛
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.
boo, i'll clean this up :)
public readonly r?: BN | ||
public readonly s?: BN | ||
|
||
public static fromTxData(txData: TxData, common?: Common) { |
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.
Yes, would say so, please also put TxOptions being the last parameter for the constructor for consistency (unless you have got an explicit reason not to).
Yeah, sorry for un-simplify here a bit, but I think this is quite a realistic scenario that we want to add here at some point and it would be a bit a pity if this would be blocked by not have this prepared.
} | ||
|
||
/** | ||
* sign a transaction with a given private key | ||
* Sign a transaction with a given private key | ||
* @param privateKey - Must be 32 bytes in length | ||
*/ | ||
sign(privateKey: Buffer) { |
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 think we should point this out more clearly in the code docs that this only gives a signed transaction if assigned to a new variable, otherwise this is too dangerous that people assume their transaction to be signed when just using tx.sign()
(and not further thinking about immutability, which is in fact rather likely than unlikely I would assume).
🎉 |
* block -> refactor: reworked header class with static factory instantiation helpers, removed defineProperties usage, fixed header tests * block -> refactoring: added new static factory helpers to block class * block -> refactor: fix build errors, remove unused imports, unpad number-value buffers in header * block -> rename Header to BlockHeader * block/tx -> fix block tests * block -> enforce BNs on fields which are interpreted as numbers * block -> edge case in toBN block -> fix tests, fix util * ethash -> make ethash compatible with block * have validateTransactions return a string[] (#812 (comment)) * let => const * set default param to resolve js runtime check * continue refactoring and simplifying methods freeze both block and header objects use Address for coinbase * api updates * continuing work * inline buffer validations. add checks for extraData, mixHash and nonce * various fixups * continuing various work * continuing work and refactoring added Block.genesis() and BlockHeader.genesis() alias update vm * re-add timestamp to genesis (for rinkeby) * last fixups * update readme, benchmarks * update vm readme, simplify validate * fix timestamp validation * use native eq * make blockchain optional in block.validate() move genTxTrie() inside validateTransactionsTrie() * fixups * remove BLOCK_difficulty_GivenAsList from skip list (#883 (comment)) Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com> Co-authored-by: Ryan Ghods <ryan@ryanio.com>
This PR continues the efforts from ethereumjs/ethereumjs-tx#154, applying it to the monorepo.
TL;DR: For a full description of all the changes, please see the tx changelog.
This PR:
ethereumjs-util
'sdefineProperties
(see Design problems and a proposal to fix them organization#56)Buffer
sfromTxData
,fromRlpSerializedTx
, andfromValuesArray
Common
, instead of string options for chain and hardforkThis PR also removes FakeTransaction as I found it burdensome to maintain in this refactor due to
from
not being stored anymore. Its only use was in block'sfrom-rpc
and can achieve the same result on the normal Transaction class with the same code there, by overridinggetSenderAddress
andhash
, see here and the note in the readme.Examples of updating code
Initializing with object: