Merge pull request #1089 from ethereumjs/block-tx-copy-common

holgerd77 · web-flow · commit 0df1f45af6bc · 2021-02-11T08:58:19.000+01:00
Block, Tx: always copy Common on instantiation / Fix HF consistency bugs
diff --git a/packages/block/src/block.ts b/packages/block/src/block.ts
@@ -25,7 +25,11 @@ export class Block {
     // parse transactions
     const transactions = []
     for (const txData of txsData || []) {
-      const tx = Transaction.fromTxData(txData, opts as TxOptions)
+      const tx = Transaction.fromTxData(txData, {
+        ...opts,
+        // Use header common in case of hardforkByBlockNumber being activated
+        common: header._common,
+      } as TxOptions)
       transactions.push(tx)
     }
 
@@ -34,7 +38,10 @@ export class Block {
     for (const uhData of uhsData || []) {
       const uh = BlockHeader.fromHeaderData(uhData, {
         ...opts,
-        // Disable this option here (all other options carried over), since this overwrites the provided Difficulty to an incorrect value
+        // Use header common in case of hardforkByBlockNumber being activated
+        common: header._common,
+        // Disable this option here (all other options carried over), since this overwrites
+        // the provided Difficulty to an incorrect value
         calcDifficultyFromHeader: undefined,
       })
       uncleHeaders.push(uh)
@@ -65,7 +72,13 @@ export class Block {
     // parse transactions
     const transactions = []
     for (const txData of txsData || []) {
-      transactions.push(Transaction.fromValuesArray(txData, opts))
+      transactions.push(
+        Transaction.fromValuesArray(txData, {
+          ...opts,
+          // Use header common in case of hardforkByBlockNumber being activated
+          common: header._common,
+        })
+      )
     }
 
     // parse uncle headers
@@ -74,6 +87,8 @@ export class Block {
       uncleHeaders.push(
         BlockHeader.fromValuesArray(uncleHeaderData, {
           ...opts,
+          // Use header common in case of hardforkByBlockNumber being activated
+          common: header._common,
           // Disable this option here (all other options carried over), since this overwrites the provided Difficulty to an incorrect value
           calcDifficultyFromHeader: undefined,
         })
diff --git a/packages/block/src/header.ts b/packages/block/src/header.ts
@@ -173,7 +173,10 @@ export class BlockHeader {
     options: BlockOptions = {}
   ) {
     if (options.common) {
-      this._common = options.common
+      this._common = Object.assign(
+        Object.create(Object.getPrototypeOf(options.common)),
+        options.common
+      )
     } else {
       const chain = 'mainnet' // default
       if (options.initWithGenesisHeader) {
@@ -685,6 +688,10 @@ export class BlockHeader {
   cliqueSigner(): Address {
     this._requireClique('cliqueSigner')
     const extraSeal = this.cliqueExtraSeal()
+    // Reasonable default for default blocks
+    if (extraSeal.length === 0) {
+      return Address.zero()
+    }
     const r = extraSeal.slice(0, 32)
     const s = extraSeal.slice(32, 64)
     const v = bufferToInt(extraSeal.slice(64, 65)) + 27
diff --git a/packages/block/src/types.ts b/packages/block/src/types.ts
@@ -13,6 +13,9 @@ export interface BlockOptions {
   /**
    * A Common object defining the chain and the hardfork a block/block header belongs to.
    *
+   * Object will be internally copied so that tx behavior don't incidentally
+   * change on future HF changes.
+   *
    * Default: `Common` object set to `mainnet` and the HF currently defined as the default
    * hardfork in the `Common` class.
    *
diff --git a/packages/block/test/clique.spec.ts b/packages/block/test/clique.spec.ts
@@ -80,7 +80,7 @@ tape('[Header]: Clique PoA Functionality', function (t) {
   t.test('Signing', function (st) {
     const cliqueSigner = A.privateKey
 
-    const header = BlockHeader.fromHeaderData(
+    let header = BlockHeader.fromHeaderData(
       { number: 1, extraData: Buffer.alloc(97) },
       { common, freeze: false, cliqueSigner }
     )
@@ -89,6 +89,13 @@ tape('[Header]: Clique PoA Functionality', function (t) {
     st.ok(header.cliqueVerifySignature([A.address]), 'should verify signature')
     st.ok(header.cliqueSigner().equals(A.address), 'should recover the correct signer address')
 
+    header = BlockHeader.fromHeaderData({}, { common })
+    st.deepEqual(
+      header.cliqueSigner(),
+      Address.zero(),
+      'should return zero address on default block'
+    )
+
     st.end()
   })
 })
diff --git a/packages/block/test/header.spec.ts b/packages/block/test/header.spec.ts
@@ -41,6 +41,14 @@ tape('[Block]: Header functions', function (t) {
     const common = new Common({ chain: 'ropsten', hardfork: 'chainstart' })
     let header = BlockHeader.genesis(undefined, { common })
     st.ok(header.hash().toString('hex'), 'genesis block should initialize')
+    st.equal(header._common.hardfork(), 'chainstart', 'should initialize with correct HF provided')
+
+    common.setHardfork('byzantium')
+    st.equal(
+      header._common.hardfork(),
+      'chainstart',
+      'should stay on correct HF if outer common HF changes'
+    )
 
     header = BlockHeader.fromHeaderData({}, { common })
     st.ok(header.hash().toString('hex'), 'default block should initialize')
diff --git a/packages/blockchain/src/db/manager.ts b/packages/blockchain/src/db/manager.ts
@@ -167,7 +167,7 @@ export class DBManager {
       }
     }
     const blockData = [header, ...body] as BlockBuffer
-    const opts = { common: this._common }
+    const opts = { common: this._common, hardforkByBlockNumber: true }
     return Block.fromValuesArray(blockData, opts)
   }
 
@@ -184,7 +184,7 @@ export class DBManager {
    */
   async getHeader(blockHash: Buffer, blockNumber: BN) {
     const encodedHeader = await this.get(DBTarget.Header, { blockHash, blockNumber })
-    const opts = { common: this._common }
+    const opts = { common: this._common, hardforkByBlockNumber: true }
     return BlockHeader.fromRLPSerializedHeader(encodedHeader, opts)
   }
 
diff --git a/packages/blockchain/src/index.ts b/packages/blockchain/src/index.ts
@@ -202,11 +202,10 @@ export default class Blockchain implements BlockchainInterface {
   public static async fromBlocksData(blocksData: BlockData[], opts: BlockchainOptions = {}) {
     const blockchain = await Blockchain.create(opts)
     for (const blockData of blocksData) {
-      const common = Object.assign(
-        Object.create(Object.getPrototypeOf(blockchain._common)),
-        blockchain._common
-      )
-      const block = Block.fromBlockData(blockData, { common, hardforkByBlockNumber: true })
+      const block = Block.fromBlockData(blockData, {
+        common: blockchain._common,
+        hardforkByBlockNumber: true,
+      })
       await blockchain.putBlock(block)
     }
     return blockchain
@@ -798,7 +797,10 @@ export default class Blockchain implements BlockchainInterface {
     await this.runWithLock<void>(async () => {
       const block =
         item instanceof BlockHeader
-          ? new Block(item, undefined, undefined, { common: this._common })
+          ? new Block(item, undefined, undefined, {
+              common: this._common,
+              hardforkByBlockNumber: true,
+            })
           : item
       const isGenesis = block.isGenesis()
 
diff --git a/packages/client/lib/blockchain/chain.ts b/packages/client/lib/blockchain/chain.ts
@@ -267,7 +267,10 @@ export class Chain extends EventEmitter {
     }
     await this.open()
     blocks = blocks.map((b: Block) =>
-      Block.fromValuesArray(b.raw(), { common: this.config.chainCommon })
+      Block.fromValuesArray(b.raw(), {
+        common: this.config.chainCommon,
+        hardforkByBlockNumber: true,
+      })
     )
     await this.blockchain.putBlocks(blocks)
     await this.update()
@@ -302,7 +305,10 @@ export class Chain extends EventEmitter {
     }
     await this.open()
     headers = headers.map((h) =>
-      BlockHeader.fromValuesArray(h.raw(), { common: this.config.chainCommon })
+      BlockHeader.fromValuesArray(h.raw(), {
+        common: this.config.chainCommon,
+        hardforkByBlockNumber: true,
+      })
     )
     await this.blockchain.putHeaders(headers)
     await this.update()
diff --git a/packages/client/lib/net/protocol/ethprotocol.ts b/packages/client/lib/net/protocol/ethprotocol.ts
@@ -63,9 +63,14 @@ export class EthProtocol extends Protocol {
       name: 'BlockHeaders',
       code: 0x04,
       encode: (headers: BlockHeader[]) => headers.map((h) => h.raw()),
-      decode: (headers: BlockHeaderBuffer[]) =>
-        /* eslint-disable-next-line no-invalid-this */
-        headers.map((h) => BlockHeader.fromValuesArray(h, { common: this.config.chainCommon })),
+      decode: (headers: BlockHeaderBuffer[]) => {
+        return headers.map((h) =>
+          BlockHeader.fromValuesArray(h, {
+            hardforkByBlockNumber: true,
+            common: this.config.chainCommon, // eslint-disable-line no-invalid-this
+          })
+        )
+      },
     },
     {
       name: 'GetBlockBodies',
diff --git a/packages/client/lib/net/protocol/lesprotocol.ts b/packages/client/lib/net/protocol/lesprotocol.ts
@@ -91,10 +91,12 @@ export class LesProtocol extends Protocol {
       decode: ([reqId, bv, headers]: any) => ({
         reqId: new BN(reqId),
         bv: new BN(bv),
-        headers: headers.map((h: BlockHeaderBuffer) =>
-          /* eslint-disable-next-line no-invalid-this */
-          BlockHeader.fromValuesArray(h, { common: this.config.chainCommon })
-        ),
+        headers: headers.map((h: BlockHeaderBuffer) => {
+          return BlockHeader.fromValuesArray(h, {
+            hardforkByBlockNumber: true,
+            common: this.config.chainCommon, // eslint-disable-line no-invalid-this
+          })
+        }),
       }),
     },
   ]
diff --git a/packages/client/lib/sync/execution/vmexecution.ts b/packages/client/lib/sync/execution/vmexecution.ts
@@ -152,7 +152,9 @@ export class VMExecution extends Execution {
             // error can repeatedly processed for debugging
             const blockNumber = block.header.number.toNumber()
             const hash = short(block.hash())
-            this.config.logger.warn(`Execution of block number=${blockNumber} hash=${hash} failed`)
+            this.config.logger.warn(
+              `Execution of block number=${blockNumber} hash=${hash} hardfork=${this.hardfork} failed`
+            )
             this.emit('error', error)
             errorBlock = block
           }
diff --git a/packages/client/lib/sync/fetcher/blockfetcher.ts b/packages/client/lib/sync/fetcher/blockfetcher.ts
@@ -1,4 +1,4 @@
-import { Block, BlockBodyBuffer } from '@ethereumjs/block'
+import { Block, BlockBodyBuffer, BlockBuffer } from '@ethereumjs/block'
 import { Peer } from '../../net/peer'
 import { EthProtocolMethods } from '../../net/protocol'
 import { Job } from './types'
@@ -50,11 +50,14 @@ export class BlockFetcher extends BlockFetcherBase<Block[], Block> {
     const bodies: BlockBodyBuffer[] = <BlockBodyBuffer[]>(
       await peer!.eth!.getBlockBodies(headers.map((h) => h.hash()))
     )
-    const blocks: Block[] = bodies.map(([txsData, unclesData]: BlockBodyBuffer, i: number) =>
-      Block.fromValuesArray([headers[i].raw(), txsData, unclesData], {
+    const blocks: Block[] = bodies.map(([txsData, unclesData]: BlockBodyBuffer, i: number) => {
+      const opts = {
         common: this.config.chainCommon,
-      })
-    )
+        hardforkByBlockNumber: true,
+      }
+      const values: BlockBuffer = [headers[i].raw(), txsData, unclesData]
+      return Block.fromValuesArray(values, opts)
+    })
     return blocks
   }
 
diff --git a/packages/tx/src/transaction.ts b/packages/tx/src/transaction.ts
@@ -110,7 +110,7 @@ export default class Transaction {
     }
 
     if (opts?.common) {
-      this.common = opts.common
+      this.common = Object.assign(Object.create(Object.getPrototypeOf(opts.common)), opts.common)
     } else {
       const DEFAULT_CHAIN = 'mainnet'
       this.common = new Common({ chain: DEFAULT_CHAIN })
diff --git a/packages/tx/src/types.ts b/packages/tx/src/types.ts
@@ -8,6 +8,9 @@ export interface TxOptions {
   /**
    * A Common object defining the chain and hardfork for the transaction.
    *
+   * Object will be internally copied so that tx behavior don't incidentally
+   * change on future HF changes.
+   *
    * Default: `Common` object set to `mainnet` and the default hardfork as defined in the `Common` class.
    *
    * Current default hardfork: `istanbul`
diff --git a/packages/tx/test/api.ts b/packages/tx/test/api.ts
@@ -24,6 +24,17 @@ tape('[Transaction]: Basic functions', function (t) {
     st.equal(tx.common.hardfork(), 'istanbul', 'should initialize with correct default HF')
     st.ok(Object.isFrozen(tx), 'tx should be frozen by default')
 
+    const common = new Common({ chain: 'mainnet', hardfork: 'spuriousDragon' })
+    tx = Transaction.fromTxData({}, { common })
+    st.equal(tx.common.hardfork(), 'spuriousDragon', 'should initialize with correct HF provided')
+
+    common.setHardfork('byzantium')
+    st.equal(
+      tx.common.hardfork(),
+      'spuriousDragon',
+      'should stay on correct HF if outer common HF changes'
+    )
+
     tx = Transaction.fromTxData({}, { freeze: false })
     st.ok(!Object.isFrozen(tx), 'tx should not be frozen when freeze deactivated in options')
 
diff --git a/packages/vm/README.md b/packages/vm/README.md
@@ -177,10 +177,12 @@ The following loggers are currently available:
 
 | Logger | Description |
 | - | - |
+| `vm:block` | Block operations (run txs, generating receipts, block rewards,...)
 | `vm:tx` | Transaction operations (account updates, checkpointing,...) |
 | `vm:tx:gas` | Transaction gas logger |
 | `vm:evm` | EVM control flow, CALL or CREATE message execution |
 | `vm:evm:gas` | EVM gas logger |
+| `vm:state`| StateManager logger |
 | `vm:ops` | Opcode traces |
 | `vm:ops:[Lower-case opcode name]` | Traces on a specific opcode |
 
@@ -198,6 +200,12 @@ Run all loggers currently available:
 DEBUG=vm:*,vm:*:* ts-node test.ts
 ```
 
+Excluding the state logger:
+
+```shell
+DEBUG=vm:*,vm:*:*,-vm:state ts-node test.ts
+```
+
 Run some specific loggers including a logger specifically logging the `SSTORE` executions from the VM (this is from the screenshot above):
 
 ```shell
diff --git a/packages/vm/lib/evm/eei.ts b/packages/vm/lib/evm/eei.ts
@@ -262,7 +262,13 @@ export default class EEI {
   getBlockCoinbase(): BN {
     let coinbase: Address
     if (this._common.consensusAlgorithm() === 'clique') {
-      coinbase = this._env.block.header.cliqueSigner()
+      // Backwards-compatibilty check
+      // TODO: can be removed along VM v5 release
+      if ('cliqueSigner' in this._env.block.header) {
+        coinbase = this._env.block.header.cliqueSigner()
+      } else {
+        coinbase = Address.zero()
+      }
     } else {
       coinbase = this._env.block.header.coinbase
     }
diff --git a/packages/vm/lib/evm/evm.ts b/packages/vm/lib/evm/evm.ts
@@ -209,8 +209,16 @@ export default class EVM {
 
     // Load code
     await this._loadCode(message)
-    if (!message.code || message.code.length === 0 || errorMessage) {
-      debug(`Exit early on no code or value tranfer overflowed`)
+    let exit = false
+    if (!message.code || message.code.length === 0) {
+      exit = true
+      debug(`Exit early on no code`)
+    }
+    if (errorMessage) {
+      exit = true
+      debug(`Exit early on value tranfer overflowed`)
+    }
+    if (exit) {
       return {
         gasUsed: new BN(0),
         execResult: {
@@ -287,8 +295,16 @@ export default class EVM {
       errorMessage = e
     }
 
-    if (!message.code || message.code.length === 0 || errorMessage) {
-      debug(`Exit early on no code or value tranfer overflowed`)
+    let exit = false
+    if (!message.code || message.code.length === 0) {
+      exit = true
+      debug(`Exit early on no code`)
+    }
+    if (errorMessage) {
+      exit = true
+      debug(`Exit early on value tranfer overflowed`)
+    }
+    if (exit) {
       return {
         gasUsed: new BN(0),
         createdAddress: message.to,
@@ -478,7 +494,9 @@ export default class EVM {
   async _reduceSenderBalance(account: Account, message: Message): Promise<void> {
     account.balance.isub(message.value)
     const result = this._state.putAccount(message.caller, account)
-    debug(`Reduce sender (${message.caller.toString()}) balance (-> ${account.balance.toString()})`)
+    debug(
+      `Reduced sender (${message.caller.toString()}) balance (-> ${account.balance.toString()})`
+    )
     return result
   }
 
@@ -490,7 +508,7 @@ export default class EVM {
     toAccount.balance = newBalance
     // putAccount as the nonce may have changed for contract creation
     const result = this._state.putAccount(message.to, toAccount)
-    debug(`Add toAccount (${message.to.toString()}) balance (-> ${toAccount.balance.toString()})`)
+    debug(`Added toAccount (${message.to.toString()}) balance (-> ${toAccount.balance.toString()})`)
     return result
   }
 
diff --git a/packages/vm/lib/runBlock.ts b/packages/vm/lib/runBlock.ts
diff --git a/packages/vm/lib/runTx.ts b/packages/vm/lib/runTx.ts
diff --git a/packages/vm/lib/state/stateManager.ts b/packages/vm/lib/state/stateManager.ts
diff --git a/packages/vm/tests/api/runBlock.spec.ts b/packages/vm/tests/api/runBlock.spec.ts

Original file line number	Diff line number	Diff line change
`@@ -13,6 +13,9 @@ export interface BlockOptions {`
`13`	`13`	`/**`
`14`	`14`	`* A Common object defining the chain and the hardfork a block/block header belongs to.`
`15`	`15`	`*`
	`16`	`+ * Object will be internally copied so that tx behavior don't incidentally`
	`17`	`+ * change on future HF changes.`
	`18`	`+ *`
`16`	`19`	* Default: `Common` object set to `mainnet` and the HF currently defined as the default
`17`	`20`	* hardfork in the `Common` class.
`18`	`21`	`*`
Original file line number	Diff line number	Diff line change
`@@ -167,7 +167,7 @@ export class DBManager {`
`167`	`167`	`}`
`168`	`168`	`}`
`169`	`169`	`const blockData = [header, ...body] as BlockBuffer`
`170`		`- const opts = { common: this._common }`
	`170`	`+ const opts = { common: this._common, hardforkByBlockNumber: true }`
`171`	`171`	`return Block.fromValuesArray(blockData, opts)`
`172`	`172`	`}`
`173`	`173`
`@@ -184,7 +184,7 @@ export class DBManager {`
`184`	`184`	`*/`
`185`	`185`	`async getHeader(blockHash: Buffer, blockNumber: BN) {`
`186`	`186`	`const encodedHeader = await this.get(DBTarget.Header, { blockHash, blockNumber })`
`187`		`- const opts = { common: this._common }`
	`187`	`+ const opts = { common: this._common, hardforkByBlockNumber: true }`
`188`	`188`	`return BlockHeader.fromRLPSerializedHeader(encodedHeader, opts)`
`189`	`189`	`}`
`190`	`190`
Original file line number	Diff line number	Diff line change
`@@ -267,7 +267,10 @@ export class Chain extends EventEmitter {`
`267`	`267`	`}`
`268`	`268`	`await this.open()`
`269`	`269`	`blocks = blocks.map((b: Block) =>`
`270`		`- Block.fromValuesArray(b.raw(), { common: this.config.chainCommon })`
	`270`	`+ Block.fromValuesArray(b.raw(), {`
	`271`	`+ common: this.config.chainCommon,`
	`272`	`+ hardforkByBlockNumber: true,`
	`273`	`+ })`
`271`	`274`	`)`
`272`	`275`	`await this.blockchain.putBlocks(blocks)`
`273`	`276`	`await this.update()`
`@@ -302,7 +305,10 @@ export class Chain extends EventEmitter {`
`302`	`305`	`}`
`303`	`306`	`await this.open()`
`304`	`307`	`headers = headers.map((h) =>`
`305`		`- BlockHeader.fromValuesArray(h.raw(), { common: this.config.chainCommon })`
	`308`	`+ BlockHeader.fromValuesArray(h.raw(), {`
	`309`	`+ common: this.config.chainCommon,`
	`310`	`+ hardforkByBlockNumber: true,`
	`311`	`+ })`
`306`	`312`	`)`
`307`	`313`	`await this.blockchain.putHeaders(headers)`
`308`	`314`	`await this.update()`