vm: add tx receipt to RunTxResult, allow BlockBuilder to build a block with zero txs #1185

ryanio · 2021-04-05T20:54:21Z

This PR:

Adds receipt to RunTxResult
- Moves the tx receipt generation logic from runBlock to runTx
- Requested by @alcuadrado
Fixes BlockBuilder to allow building a block with zero txs
AccessListEIP2930Transaction: adds alias type for transactionType so it can be interpreted correctly when passed to fromTxData
TransactionFactory: For extra safety, adds toBuffer on txData.type in case a 0x-prefixed hex string is passed in
Adds types test from @alcuadrado to ensure that the actual objects can be safely used as their data types
Adds test cases for legacy and access list transactions to runBlock
BlockBuilder: Moves the stateManager.commit to after putting the block in blockchain in case it throws on validating

codecov · 2021-04-05T20:56:08Z

Codecov Report

Merging #1185 (582ed17) into master (36e7396) will decrease coverage by 0.18%.
The diff coverage is 82.14%.

Flag	Coverage Δ
block	`82.25% <ø> (+0.20%)`	⬆️
blockchain	`84.23% <ø> (+0.06%)`	⬆️
client	`84.03% <ø> (ø)`
common	`87.39% <ø> (ø)`
devp2p	`83.96% <ø> (-0.17%)`	⬇️
ethash	`82.08% <ø> (ø)`
tx	`81.98% <61.53%> (+0.22%)`	⬆️
vm	`81.97% <88.37%> (-1.67%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

packages/block/src/types.ts

packages/tx/src/transactionFactory.ts

packages/vm/lib/runBlock.ts

packages/vm/lib/runTx.ts

alcuadrado · 2021-04-06T00:18:49Z

I'd also add a test that does runBlock with a legacy and an access list transaction. That would help ensure that the problem with recreating txs doesn't come back.

packages/vm/lib/runBlock.ts

holgerd77 · 2021-04-06T08:35:45Z

Updated the branch here (have tested locally with a rebase before).

holgerd77

Great PR in general 😀 , have some further remarks though which needs to be addressed.

Realistically I think it is too rushed to still do a release here (and also including the tx bug reports) still until the end of the week, but we can target early next week for a new release.

packages/tx/src/eip2930Transaction.ts

packages/vm/lib/runBlock.ts

packages/vm/lib/runTx.ts

packages/vm/tests/api/types.spec.ts

packages/vm/lib/runBlock.ts

holgerd77 · 2021-04-08T10:43:23Z

packages/vm/tests/api/runBlock.spec.ts

+    await simpleRun(vm, [tx])
+    t.end()
+  })
+})


So, for the release notes: did runBlock() previously not work with typed txs or are these just additional tests for safety reasons?

there was a bug in the block constructor would recreate a AccessListEIP2930Transaction as a legacy transaction (due to the missing type field, solved by the newly introduced alias). these tests were added to ensure the bug was fixed, and so it won't be reintroduced in the future (#1185 (comment))

packages/vm/lib/buildBlock.ts

holgerd77 · 2021-04-08T10:59:49Z

Update: ah, took a step back here. Maybe we can do a release tomorrow, but very much depends on the subsequent discussion and decisions here.

Generally #1189 is a lot more pressing, in doubt we should do a bugfix release in between tomorrow.

… in as a 0x-prefixed string

… for `fromTxData` and creates the correct tx

…ockchain in case it throws on validating

…data (adds coverage for `type` property alias)

* add `_type` and aliases * move EIP-2930 aliases for senderS, senderR, yParity so they can be available for all transactions

…ctory

holgerd77

LGTM, thanks a lot! 😄

packages/tx/src/baseTransaction.ts

ryanio added PR state: needs review package: vm labels Apr 5, 2021

ryanio added PR state: WIP package: block package: tx labels Apr 5, 2021

ryanio force-pushed the vm-updates branch 2 times, most recently from 6f34850 to 89f5ee2 Compare April 5, 2021 23:12

ryanio removed the PR state: WIP label Apr 5, 2021