Block library refactoring #883
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Ok, have now also pushed the changes with the static factory methods to the block library, some fixes left and tests are not passing yet. Will stop for today, if someone wants to continue during the day feel free to pick up, then please leave a short note here. Also feel free to evolve on all the design decisions made. |
|
Will pick up here after some research regarding this number padding and partially solve #726 |
…ation helpers, removed defineProperties usage, fixed header tests
…ber-value buffers in header
jochem-brouwer
force-pushed
the
refactor-block-library
branch
from
96318fa to
dd228bc
Compare
|
Rebased and fixed the build errors. The CI is very likely to fail. This was much more complex than I anticipated. Some stuff which I noticed (might not be related to this PR): Why are we using FakeTx in It seems that we are making Also related to read-only: while the Header might me read-only I think you can still edit the Buffers themselves, for instance by slicing the Buffer and then writing to this Buffer (points to same memory area). We might consider (maybe in a follow-up PR) to create read only buffers? @s1na Will stop here for today. |
|
Hi @jochem-brouwer, great that you've picked up on this, I think these kind of PRs benefit if work on this is shared since input on design decisions can be much more diverse and in discussion! 👍 😄 Some notes on what I noticed during my initial implementation part:
Ok, so far. Will do some TypeScript transition work on the client today (just to let you know so that we won't be doing any parallel double work). 😄 |
|
Will leave a note if/when I continue. Another note: before we merge this one, |
|
Will continue here. |
|
awesome work here guys. @jochem-brouwer do you want to update the vars you identified as numbers from Buffer to BN? (gasLimit, difficulty, gasUsed, number, timestamp) |
freeze both block and header objects use Address for coinbase
|
i am finished for today but will pick up again tomorrow morning 👍 everything seems to be coming along nicely. |
added Block.genesis() and BlockHeader.genesis() alias update vm
ryanio
added
PR state: merge ready
PR state: needs review
package: vm
and removed
PR state: WIP
labels
move genTxTrie() inside validateTransactionsTrie()
|
Will do a review here now. Totally loved this "three people worked on the same PR seamlessly" work style here. 👍 😄 |
There was a problem hiding this comment.
wow, wow, wow.
What a PR! 🙂
Can confirm that this looks great! 👍 🎉
Would give this a go, but please give this Common immutability question a dedicated look.
Please also wait with merging, I was settling out with @evertonfraga how we do the process here in relation to #886 (respectively you guys can of course also settle directly, we should just exchange on the merge -> rebase order here before we proceed).
Note: just realize that I actually can't approve myself anyhow since I am the original PR author, lol 😛 , so take this as an informal approval and feel free to approve yourself.
| @@ -184,7 +184,7 @@ export class Block { | |||
| } | |||
| }) | |||
|
|
|||
| return stringError ? errors.join(' ') : errors.length === 0 | |||
| return stringError ? errors : errors.length === 0 | |||
There was a problem hiding this comment.
Yeah, just re-read the discussion with @alcuadrado, this really makes a lot of sense to skip the join here.
| if (txErrors !== '') { | ||
| throw new Error(txErrors) | ||
| if (txErrors.length > 0) { | ||
| throw new Error(`invalid transactions: ${txErrors.join(' ')}`) |
| opts: BlockOptions = {}, | ||
| ) { | ||
| this.header = header | ||
| this.transactions = transactions | ||
| this.uncleHeaders = uncleHeaders | ||
| this._common = this.header._common | ||
|
|
||
| Object.freeze(this) |
There was a problem hiding this comment.
For understanding on further implications: does this Object.freeze() mean we can't do further modifications on e.g. the Common instance passed?
So would a subsequent call (e.g. in a VM context) common.setHardfork('byzantium') (or whatever) throw in this context or would this still work?
There was a problem hiding this comment.
I'll have to give it a test, but I think the common object can still update, it just can't be set to a totally new instance (e.g. block._common = newCommon would fail)
There was a problem hiding this comment.
Yes I think this is the case according to here:
Note that values that are objects can still be modified, unless they are also frozen.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze
packages/block/src/block.ts
Outdated
|
|
||
| return rlpEncode ? rlp.encode(raw) : raw | ||
| serialize(): Buffer { | ||
| return rlp.encode(this.raw) |
There was a problem hiding this comment.
Whew, such a strange call mixture with the raw() method before here. This is so much cleaner decoupled, great!
| stateRoot ? checkBufferLength(toBuffer(stateRoot), 32) : zeros(32), | ||
| transactionsTrie ? checkBufferLength(toBuffer(transactionsTrie), 32) : KECCAK256_RLP, | ||
| receiptTrie ? checkBufferLength(toBuffer(receiptTrie), 32) : KECCAK256_RLP, | ||
| coinbase ? new Address(toBuffer(coinbase)) : Address.zero(), |
There was a problem hiding this comment.
Super beautiful to see this new Address class in action in places like this now! 😀
| public static genesis(blockData: BlockData = {}, opts: BlockOptions = {}) { | ||
| opts = { ...opts, initWithGenesisHeader: true } | ||
| return Block.fromBlockData(blockData, opts) | ||
| } |
There was a problem hiding this comment.
Ah, thought we can really keep this as simple as you proposed (or more or less), so optimally Block.genesis(common), and everyone needing something adjusted can just use the Block.fromBlockData() variant?
There was a problem hiding this comment.
Yeah I wanted to too but in practice (in our test suite) it was helpful to accept custom overrides as well.
| public static genesis(headerData: HeaderData = {}, opts: BlockOptions = {}) { | ||
| opts = { ...opts, initWithGenesisHeader: true } | ||
| return BlockHeader.fromHeaderData(headerData, opts) | ||
| } |
| @@ -6,16 +6,16 @@ import { Block } from '../src' | |||
| tape('[Block]: block functions', function (t) { | |||
| t.test('should test block initialization', function (st) { | |||
| const common = new Common({ chain: 'ropsten', hardfork: 'chainstart' }) | |||
| const block1 = Block.fromBlockData({}, { common: common, initWithGenesisHeader: true }) | |||
| st.ok(block1.hash().toString('hex'), 'block should initialize') | |||
| const genesis = Block.genesis({}, { common }) | |||
There was a problem hiding this comment.
Ah, on the other hand (on the signature question): this also looks pretty simple already, so maybe not worth the further simplification, not sure.
| const genesisBlock = new Block([header.raw, [], []], { common }) | ||
| await blockchain.putGenesis(genesisBlock) | ||
| const header = testData.genesisBlockHeader | ||
| const genesis = Block.genesis({ header }, { common }) |
There was a problem hiding this comment.
scrolls down...
scrolls down...
Phew, I really wasn't aware that there were so many references / usages of this genesis functionality. 😃
| @@ -48,7 +47,7 @@ tape('runBlockchain', (t) => { | |||
| t.test('should run with genesis block', async (st) => { | |||
| try { | |||
| const common = new Common({ chain: 'goerli', hardfork: 'chainstart' }) | |||
| const genesis = createGenesis({ common }) | |||
| const genesis = Block.genesis(undefined, { common }) | |||
There was a problem hiding this comment.
Why this undefined here instead of {}?
There was a problem hiding this comment.
Both work since the param defaults to {} if undefined.
|
Update: ok, @evertonfraga has given his ok for the merge here. 😊 |
This PR is a start on the block library refactoring, going very much along the structure introduced along #812 on the tx library.
WIP, non-block-dependent header tests pass, on the first refactoring round I kept values as Buffers and didn't adopt to domain-specific value representations as in the tx library refactor. This can be changed upon subsequent commits.
Noteworthy changes on the first commit:
genesisheadernumberBuffer from being the empty Buffer to 0-Buffer, for RLP serialization this is now unpaddednumberBuffer from this legacy homestead Buffer to the empty Buffer (this can be discussed)