Breaking Releases - Meta Issue (VM v6, others) (old)

[holgerd77]

NOTE: ISSUE HAS BEEN REPLACED BY NEW ISSUES #1717 AND #1718

Follow-up on #907
Work currently done towards: develop (we might switch to master at some point)

Release planning for the next major releases (VM v6 and other major releases), please use this to add breaking changes planned as well as deprecation tasks which should be executed upon.

Planned Release Date (Timeframe): Latest somewhat before Merge HF (~Summer 2022), eventually earier

Breaking Changes / Deprecation Tasks

Ecosystem (updated: 2022-01-13)

• important, not-urgent: 2-3 day team sprint for error messages -> enums
  • -> Can be done on a per-library basis
  • -> 2022-01-13, @holgerd77: concrete approach not clear, some non-breaking pre-work done, needs discussion
• needs discussion + evaluation: Switch to BigInt, Monorepo -> BN.js to BigInt: Performance Analysis #1651
  • -> 2022-01-13, @holgerd77: Scope not yet defined, we will do some first trials like switching the VM stack implementation from storing BN.js instances to BigInt and see how it goes (and performes)

Various Libraries (updated: 2022-01-13)

• Make all constructors from libraries with async constructor methods (VM, Blockchain, Block, Tx ?) protected, needs strong discussion due to potential inheritance issues
  • -> Updated opinion (@holgerd77): inheritance issue would have been to grave on switching to private, protected will need some renewed discussion
• needs discussion + evaluation: Switch to peerDependencies library inclusion, see Incompatible Library versions (TypeScript errors) / Peer dependency switch #1200, some experimentation done in Add peerDependencies to packages #1402
  • -> doubts from @holgerd77, @ryanio (needs manual installation (if understood correctly), strong intervention in current installation process)
  • -> from @ryanio: npm 7 installs peerDependencies by default so this might be more reasonable to accomplish now.

VM

• somewhat important: Add StateManager to exports in main index.js file (see here for an example for the current suboptimal import referencing dist, make VM export non-default
• nice-to-have: Remove deprecated state constructor option
• somewhat important: Merge the EIP2929StateManager + Interface with the standard interface, see Fix EIP2929 #1124 for context on the introduction, also add generateAccessList() to interface and adopt StateManager and runTx()` code docs, see VM: Generate Access Lists in runTx() #1170
• somewhat important: Restructure/sort VM exports (directly expose StateManger, Bloom, other?, unified type location, type/interface exposure, index.ts -> vm.ts+ index.ts only for exports
• somewhat important: Remove deprecated generateTxReceipt function in runBlock (preferring newer version located in runTx), remove Receipt re-exports
• cannot judge, please update: Update BaseTxReceipt gasUsed from Buffer to BN (suggestion added by @ryanio)
• nice-to-have: Remove StateManager.getStateRoot() force=true parameter (also in interface)
• important: Finish up on VM, Blockchain: fix storing unsettled promises #1201 (VM, Blockchain: fix storing unsettled promises)
• somewhat important: Removed deprecated EIP2930Receipt and EIP1559Receipt types
• somewhat important: Move gasRefund to a tx-level result object instead of ExecResult todo link
• nice-to-have: Re-add VM: stateManager -> add modifyAccountFields method, PR VM: stateManager -> add modifyAccountFields method #1369
• nice-to-have: runCode throws if gasLimit is undefined: the gasLimit parameter is mandatory and should be of type BN (or at least BNLike)
• Difference between gasUsed and execResult.gasUsed #1446 (comment)

Blockchain

• nice-to-have: Consider to remove Blockchain.meta object
• somewhat important: Limit Blockchain.iterator() to return a number in function and interface, also VM.runBlockchain() and vmPromise in client full sync module
• somewhat important: Rename getHead() -> getIteratorHead() for better differentiation towards the canonical head functions (eventually also rename?)

Block

• somewhat important: Block: Encapsulate Consensus Mechanism #1044: Integrate the current ethash functionality into a new Consensus class (or similar) - yet to be integrated and some open design decisions. Discussion triggered by PoA clique implementation in Add Clique Support #1032
  • -> eventually needs more time to get a better picture on the requirements from "The Merge" implementation
• nice-to-have: Rename block field bloom to logsBloom for increased clarity (suggestion by @ryanio)
• somewhat important: Directly throw in validation functions like BlockHeader.validateGasLimit() (eventually needs some final confirmation), throw with more appropriate error messages (e.g. EIP-1559 specific), see Implement EIP1559 Fee Market + EIP3198 BaseFee #1148 (comment)
  • -> Needs more discussion and some confirmation
• important: Switch to centralized default value setting in the main constructor instead of setting in the static constructors, switch to a parameter dict for passing in values to the main constructor (instead of a long list of separate values), make BlockHeader.fromHeaderData (would need some check for Block.fromBlockData, likely as well) simply an alias for the main constructor instantiation using the same (typed) API, see changes/updates in the tx library for reference
  --> Subsidiary, move constructor from deprecated to private
• nice-to-have: Move Block validation methods to Blockchain, Blockchain/Block: move Block validation methods to Blockchain #947
  • -> Needs some re-evaluation and final decision

Transaction

• cannot judge, please update: Consistent use of BN(0) buffers. These are either empty buffers, undefined, or Buffer.from('00', 'hex'), which equals (new BN(0)).toBuffer(). Related: isSigned returning true for unsigned transactions #1041
• nice-to-have (?): Get rid of BufferLike type and enforce Buffer type.
• nice-to-have: Remove transactionType property in favor of type
• nice-to-have: Decide on v, r, s aliases for typed txs (keep? remove?)
  • -> Needs decision
• Re-evaluate tx.getMessageToSign(false) API for legacy txs, eventually it might be a way to switch to return the RLP encoded byte array, see Tx: EIP-155 not properly supported for legacy txs in raw() and serialize() methods #1278 (comment) for context, this might exclude some existing use cases though (so handle with care), eventually the updated documentation on this might also already be enough. Also check the situation for the txs with tx types (how should the method behave for typed txs?).
  • -> Own opinion (@holgerd77): I would rather keep "as is"
• Throw when hash() is called on unsigned legacy txs (so: align with behavior from typed txs), see also comment added along Block, Tx: Caching for hash method #1445 (this needs some additional analysis of the potential side effects)

Common (updated: 2022-01-13)

• nice-to-have: Make Common export non-default to ease on Node/JS related problems, see #default, restructure file layout and exports analogue to e.g. the tx library (dedicated common.ts main source file, index.ts only for re-exports), partly addressed in Common: Make Export non-default / ESM Module Support #1632
• important: Set default hardfork to 'london' (Update 2022-01-13 @holgerd77: confirmed), Common: Set Default HF to London #1657
• nice-to-have: Rename method names referring to "active" HFs with the terminology being ambiguous, see this comment Common: Add new isActivatedEIP() method #1125 (comment) (Update 2022-01-13 @holgerd77: will give this some additional thought once the below TODO has been implemented) Common: Rename/delete ambiguous active* methods #1674
• nice-to-have: Consider to remove supportedHardforks, onlySupported and onlyActive HF options, see also Deprecate supportedHardforks/onlySupported/onlyActive HF options #1274 (Update 2022-01-13 @holgerd77: confirmed to be useful)
• nice-to-have: Introduce more type safety to common any responses (chains, hardforks, params)
  • Update 2022-01-13 @holgerd77: there has been some execution on this, link to PR would need some research, work might be taken as reference, might need some discussion on execution
• nice-to-have: execute on all deprecation notes (e.g. remove deprecated Common.forCustomChain() method), Common: Execute on all Deprecation Notes for Breaking Release #1633
• nice-to-have: Remove Kovan support (including chain file), see discussion from block: Add fix for kovan nonce #1334
  • Update 2022-01-13 @holgerd77: there had been some Kovan additions lately, is this still wished?

Util

-> BREAKING RELEASE IF DECIDED TO DO BROADER BIGINT INTEGRATION

Note: the Util library should likely not get the same cadence on breaking release upgrades like the other libraries, since breaking releases take a very long time till they get sufficient adoption by the community (the Util library is just not the "we need to update this immediately" library but people are satisfied if its doing its job.

Breaking changes are nevertheless tracked here. It should be decided upon carefully though if a breaking release will be done or not.

• Remove deprecated bnToRlp() method (types)
• Move misplaced bnToHex() and bnToUnpaddedBuffer() methods from types to the bytes module
• Rename library to @ethereumjs/util
• Unify return values along signature/address: support for high recovery and chain IDs ethereumjs-util#290 for methods like ecsign and always return Buffer, remove function overloading, merge interfaces like ECDSASignature and ECDSASignatureBuffer (to: ECDSASignature)
• Check homesteadOrLater parameter in isValidSignature()
• Deprecate/remove fromRpcSig() function, see doc comment (toRpcSig()?)
• Unify const stripZeros = function (a: any): Buffer | number[] | string and bnToUnpaddedBuffer() to const stripZeros = function (a: any): BufferLike (which also encompassed BN)? (from @holgerd77, thought about this while reading over https://github.com/ethereumjs/ethereumjs-util/issues/141)
• Remove object.ts -> defineProperties()

Trie

-> LIKELY NO BREAKING RELEASE

• Replace constructor options with an options dict (like in other libraries), add an options type
• Rename from merkle-patricia-tree to @ethereumjs/trie
• Reconsider https://github.com/ethereumjs/merkle-patricia-tree/issues/108 (removal of level dependencies or alternative solution) for integration
• Remove deprecated semi-private _walkTrie() and _lookupNode() methods, see this comment along
• Refactor WalkTrie merkle-patricia-tree#135
• Make deprecated Trie.setRoot() function private (reverted in v4.1.0 release from Base trie code documentation and function clustering merkle-patricia-tree#125 with risk of breaking being too high)

[jochem-brouwer]

If we export VM as default and also export StateManager, would that be a breaking change?

[holgerd77]

Ah, seems you can do both in one file I am now concluding after googling? Didn't know that, so might also be a way. Not sure if I would not like it more to have this on the same level, but we can then always start with the named StateManager export (if we want).

[jochem-brouwer]

Have added two items to Transaction.

[holgerd77]

This list is becoming vast and I guess it's not realistic that we work this down "straight away" in 2-3 weeks but we'll rather have to implement this step-by-step and likely still need to do updates on the current library versions in parallel.

So I guess we'll likely want to - as suggested before - create some branch v6-dev or so here (what would be a good name?) and then for some time show the discipline and rebase this new branch with the changes merged into master on the published versions on a regular basis? 🤔

Should I open such branch on occasion?

We likely also want to have this branch protected I guess with more or less the same CI checks and the likes as we have on master.

[holgerd77]

(several comment updates, please read on site)

[ryanio]

v6-dev sounds good to me. yes the list is getting quite long, but most items seem to be reasonably minor level of effort.

maybe even before that we could start with some low hanging fruit like error messages -> enums which shouldn't be breaking in any way? could we open a PR like that to be merged onto master?

[holgerd77]

@ryanio ah, you are right, the error message TODO should also be possible independently from a breaking release. I always had this in my head in combination with changing the actual error strings (which then would be breaking, independently from using enums or not), which I also wanted to do for a lot of strings since many messages are not very expressive, e.g. the VM.runBlock() ones.

But we can actually separate here and introduce enums in a first step with the exact same messages and then do the error message changes on the breaking release (and then likely announce that from these releases on error messages will change along minor/bugfix releases without notice and people should compare on the enum constants (is this actually possible in JavaScript??? I guess we should really generally open an issue for this whole thing and describe what we are planning to do and give this a short discussion, @ryanio, can you eventually do directly?).

Apart from this possible split I would assume there are no further "low hanging fruits to extract" though? 🤔

One major thing I was hoping to get into v6 is actually the consensus encapsulation on Block from #1044 so that we get the library better prepared for "The Merge". Not sure if this is realistic though and if we are already "there" or if this would rather be something for v7. But just to mention.

This whole "The Merge" integration would generally need some laid back look and see what structural changes are approaching and where we likely need to break (also, e.g.: this "break point" (haha) where we have the switch from PoW to PoS can not be modelled with Common right now, funnily enough I had this in in the early Casper days (to set the consensus by HF and not by chain) and then removed 😜 ).

[tqpcharlie]

* Remove deprecated `Common.forCustomChain()` method

hello, I'd like to help with this, but can't find where to get started.

[holgerd77]

@Zachinquarantine Hi there, thanks for the offer. Preparing for breaking releases needs some very specific procedure (since we can't just merge breaking changes into master) and I guess this not so suited for third-party contributions (and in this specific case we are just not planning any Common breaking releases in the upcoming months, so we generally wouldn't tackle this until we start working on a new breaking release).

[jochem-brouwer]

RE: BigInt. By this benchmark it seems that BigInt performs far worse than BN in benchmarks. It would be an idea to just try it, but for now it seems that switching to BigInts is going to make things worse instead of better (which I honestly find very surprising if thats true).

[holgerd77]

Closing in favor of #1717 and #1718