From 4febd1899ff95235584695d13f4b0943b92656cb Mon Sep 17 00:00:00 2001 From: jangko Date: Wed, 1 Nov 2023 09:24:32 +0700 Subject: [PATCH] TxPool: Bubble up error from packer to assembleBlock --- nimbus/beacon/beacon_engine.nim | 4 ++- nimbus/core/sealer.nim | 4 ++- nimbus/core/tx_pool.nim | 21 ++++++++---- nimbus/core/tx_pool/tx_tasks/tx_packer.nim | 14 +++++--- tests/test_txpool.nim | 17 ++++++---- tests/test_txpool2.nim | 38 +++++++++++++++++++--- 6 files changed, 73 insertions(+), 25 deletions(-) diff --git a/nimbus/beacon/beacon_engine.nim b/nimbus/beacon/beacon_engine.nim index e5196eb0f..a3be4c5a0 100644 --- a/nimbus/beacon/beacon_engine.nim +++ b/nimbus/beacon/beacon_engine.nim @@ -170,7 +170,9 @@ proc generatePayload*(ben: BeaconEngineRef, # someBaseFee = true: make sure blk.header # have the same blockHash with generated payload - let blk = xp.ethBlock(someBaseFee = true) + let blk = xp.assembleBlock(someBaseFee = true).valueOr: + return err(error) + if blk.header.extraData.len > 32: return err "extraData length should not exceed 32 bytes" diff --git a/nimbus/core/sealer.nim b/nimbus/core/sealer.nim index 93015453d..591e77c94 100644 --- a/nimbus/core/sealer.nim +++ b/nimbus/core/sealer.nim @@ -61,7 +61,9 @@ proc validateSealer*(conf: NimbusConf, ctx: EthContext, chain: ChainRef): Result proc generateBlock(engine: SealingEngineRef, outBlock: var EthBlock): Result[void, string] = - outBlock = engine.txPool.ethBlock() + outBlock = engine.txPool.assembleBlock().valueOr: + return err(error) + if engine.chain.com.consensus == ConsensusType.POS: # Stop the block generator if we reach TTD engine.state = EnginePostMerge diff --git a/nimbus/core/tx_pool.nim b/nimbus/core/tx_pool.nim index f035f224d..c806f5f7e 100644 --- a/nimbus/core/tx_pool.nim +++ b/nimbus/core/tx_pool.nim @@ -241,7 +241,7 @@ ## xq.add(txs) # add transactions .. ## .. # .. into the buckets ## -## let newBlock = xq.ethBlock # fetch current mining block +## let newBlock = xq.assembleBlock # fetch current mining block ## ## .. ## mineThatBlock(newBlock) ... # external mining & signing process @@ -607,7 +607,7 @@ proc dirtyBuckets*(xp: TxPoolRef): bool = ## flag is also set. xp.pDirtyBuckets -proc ethBlock*(xp: TxPoolRef, someBaseFee: bool = false): EthBlock +proc assembleBlock*(xp: TxPoolRef, someBaseFee: bool = false): Result[EthBlock, string] {.gcsafe,raises: [CatchableError].} = ## Getter, retrieves a packed block ready for mining and signing depending ## on the internally cached block chain head, the txs in the pool and some @@ -621,18 +621,25 @@ proc ethBlock*(xp: TxPoolRef, someBaseFee: bool = false): EthBlock ## Note that this getter runs *ad hoc* all the txs through the VM in ## order to build the block. - xp.packerVmExec # updates vmState - result.header = xp.chain.getHeader # uses updated vmState + xp.packerVmExec().isOkOr: # updates vmState + return err(error) + + var blk = EthBlock( + header: xp.chain.getHeader # uses updated vmState + ) + for (_,nonceList) in xp.txDB.packingOrderAccounts(txItemPacked): - result.txs.add toSeq(nonceList.incNonce).mapIt(it.tx) + blk.txs.add toSeq(nonceList.incNonce).mapIt(it.tx) let com = xp.chain.com if com.forkGTE(Shanghai): - result.withdrawals = some(com.pos.withdrawals) + blk.withdrawals = some(com.pos.withdrawals) if someBaseFee: # make sure baseFee always has something - result.header.fee = some(result.header.fee.get(0.u256)) + blk.header.fee = some(blk.header.fee.get(0.u256)) + + ok(blk) proc gasCumulative*(xp: TxPoolRef): GasInt = ## Getter, retrieves the gas that will be burned in the block after diff --git a/nimbus/core/tx_pool/tx_tasks/tx_packer.nim b/nimbus/core/tx_pool/tx_tasks/tx_packer.nim index b9da27554..b0be72ce6 100644 --- a/nimbus/core/tx_pool/tx_tasks/tx_packer.nim +++ b/nimbus/core/tx_pool/tx_tasks/tx_packer.nim @@ -150,7 +150,7 @@ proc runTxCommit(pst: TxPackerStateRef; item: TxItemRef; gasBurned: GasInt) # Private functions: packer packerVmExec() helpers # ------------------------------------------------------------------------------ -proc vmExecInit(xp: TxPoolRef): TxPackerStateRef +proc vmExecInit(xp: TxPoolRef): Result[TxPackerStateRef, string] {.gcsafe,raises: [CatchableError].} = # Flush `packed` bucket @@ -169,14 +169,16 @@ proc vmExecInit(xp: TxPoolRef): TxPackerStateRef # EIP-4788 if xp.chain.nextFork >= FkCancun: let beaconRoot = xp.chain.com.pos.parentBeaconBlockRoot - discard xp.chain.vmState.processBeaconBlockRoot(beaconRoot) + xp.chain.vmState.processBeaconBlockRoot(beaconRoot).isOkOr: + return err(error) - TxPackerStateRef( # return value + let packer = TxPackerStateRef( # return value xp: xp, tr: newCoreDbRef(LegacyDbMemory).mptPrune, balance: xp.chain.vmState.readOnlyStateDB.getBalance(xp.chain.feeRecipient), numBlobPerBlock: 0, ) + ok(packer) proc vmExecGrabItem(pst: TxPackerStateRef; item: TxItemRef): Result[bool,void] {.gcsafe,raises: [CatchableError].} = @@ -288,14 +290,15 @@ proc vmExecCommit(pst: TxPackerStateRef) # Public functions # ------------------------------------------------------------------------------ -proc packerVmExec*(xp: TxPoolRef) {.gcsafe,raises: [CatchableError].} = +proc packerVmExec*(xp: TxPoolRef): Result[void, string] {.gcsafe,raises: [CatchableError].} = ## Rebuild `packed` bucket by selection items from the `staged` bucket ## after executing them in the VM. let db = xp.chain.com.db let dbTx = db.beginTransaction defer: dbTx.dispose() - var pst = xp.vmExecInit + var pst = xp.vmExecInit.valueOr: + return err(error) block loop: for (_,nonceList) in pst.xp.txDB.packingOrderAccounts(txItemStaged): @@ -309,6 +312,7 @@ proc packerVmExec*(xp: TxPoolRef) {.gcsafe,raises: [CatchableError].} = break account # continue with next account pst.vmExecCommit + ok() # Block chain will roll back automatically # ------------------------------------------------------------------------------ diff --git a/tests/test_txpool.nim b/tests/test_txpool.nim index 130c1d656..724c82505 100644 --- a/tests/test_txpool.nim +++ b/tests/test_txpool.nim @@ -621,7 +621,7 @@ proc runTxPackerTests(noisy = true) = # employ packer # xq.jobCommit(forceMaintenance = true) - xq.packerVmExec + check xq.packerVmExec.isOk check xq.verify.isOK # verify that the test did not degenerate @@ -647,7 +647,7 @@ proc runTxPackerTests(noisy = true) = # re-pack bucket #xq.jobCommit(forceMaintenance = true) - xq.packerVmExec + check xq.packerVmExec.isOk check xq.verify.isOK let @@ -677,7 +677,7 @@ proc runTxPackerTests(noisy = true) = # re-pack bucket, packer needs extra trigger because there is # not necessarily a buckets re-org resulting in a change #xq.jobCommit(forceMaintenance = true) - xq.packerVmExec + check xq.packerVmExec.isOk check xq.verify.isOK let @@ -736,7 +736,7 @@ proc runTxPackerTests(noisy = true) = test &"Run packer, profitability will not increase with block size": xq.flags = xq.flags - {packItemsMaxGasLimit} - xq.packerVmExec + check xq.packerVmExec.isOk let smallerBlockProfitability = xq.profitability smallerBlockSize = xq.gasCumulative @@ -748,7 +748,7 @@ proc runTxPackerTests(noisy = true) = " slack=", xq.trgGasLimit - xq.gasCumulative xq.flags = xq.flags + {packItemsMaxGasLimit} - xq.packerVmExec + check xq.packerVmExec.isOk noisy.say "***", "max-packing", " profitability=", xq.profitability, @@ -787,8 +787,13 @@ proc runTxPackerTests(noisy = true) = xq.flags = xq.flags #+ {packItemsMaxGasLimit} # Invoke packer - let blk = xq.ethBlock + let r = xq.assembleBlock() + if r.isErr: + debugEcho r.error + check false + return + let blk = r.get # Make sure that there are at least two txs on the packed block so # this test does not degenerate. check 1 < xq.chain.receipts.len diff --git a/tests/test_txpool2.nim b/tests/test_txpool2.nim index 447dbab78..4a0a0b73c 100644 --- a/tests/test_txpool2.nim +++ b/tests/test_txpool2.nim @@ -156,7 +156,13 @@ proc runTxPoolCliqueTest*() = check xp.nItems.total == 1 test "TxPool ethBlock": - blk = xp.ethBlock() + let res = xp.assembleBlock() + if res.isErr: + debugEcho res.error + check false + return + + blk = res.get body = BlockBody( transactions: blk.txs, uncles: blk.uncles @@ -192,7 +198,13 @@ proc runTxPoolCliqueTest*() = return check xp.nItems.total == 1 - blk = xp.ethBlock() + let r = xp.assembleBlock() + if r.isErr: + debugEcho r.error + check false + return + + blk = r.get body = BlockBody( transactions: blk.txs, uncles: blk.uncles @@ -241,8 +253,13 @@ proc runTxPoolPosTest*() = com.pos.feeRecipient = feeRecipient com.pos.timestamp = EthTime.now() - blk = xp.ethBlock() + let r = xp.assembleBlock() + if r.isErr: + debugEcho r.error + check false + return + blk = r.get check com.isBlockAfterTtd(blk.header) body = BlockBody( @@ -299,8 +316,13 @@ proc runTxPoolBlobhashTest*() = com.pos.feeRecipient = feeRecipient com.pos.timestamp = EthTime.now() - blk = xp.ethBlock() + let r = xp.assembleBlock() + if r.isErr: + debugEcho r.error + check false + return + blk = r.get check com.isBlockAfterTtd(blk.header) body = BlockBody( @@ -380,7 +402,13 @@ proc runTxHeadDelta*(noisy = true) = com.pos.timestamp = timestamp com.pos.feeRecipient = feeRecipient - var blk = xp.ethBlock() + let r = xp.assembleBlock() + if r.isErr: + debugEcho r.error + check false + return + + let blk = r.get check com.isBlockAfterTtd(blk.header) let body = BlockBody(