From 8073bd1815a49e509faf8aabefecc564756ed437 Mon Sep 17 00:00:00 2001 From: jangko Date: Thu, 26 Dec 2024 10:30:22 +0700 Subject: [PATCH] Fix and add more tests --- .../nodocker/engine/tx_sender.nim | 11 +- nimbus/core/tx_pool/tx_desc.nim | 50 +-- nimbus/core/tx_pool/tx_tabs.nim | 26 +- nimbus/core/validate.nim | 2 +- tests/test_txpool.nim | 351 ++++++++---------- 5 files changed, 198 insertions(+), 242 deletions(-) diff --git a/hive_integration/nodocker/engine/tx_sender.nim b/hive_integration/nodocker/engine/tx_sender.nim index 195a4a027c..bc7a971d52 100644 --- a/hive_integration/nodocker/engine/tx_sender.nim +++ b/hive_integration/nodocker/engine/tx_sender.nim @@ -95,8 +95,8 @@ proc createAccount(idx: int): TestAccount = quit(QuitFailure) result.address = toAddress(result.key) -proc createAccounts(sender: TxSender) = - for i in 0.. lifeTime: + expired.add txHash + + for txHash in expired: + xp.removeTx(txHash) + proc addTx*(xp: TxPoolRef, ptx: PooledTransaction): Result[void, TxError] = if not ptx.tx.validateChainId(xp.chain.com.chainId): return err(txErrorChainIdMismatch) @@ -276,6 +299,9 @@ proc addTx*(xp: TxPoolRef, ptx: PooledTransaction): Result[void, TxError] = if not xp.classifyValid(ptx.tx, sender): return err(txErrorTxInvalid) + if xp.idTab.len >= MAX_POOL_SIZE: + xp.removeExpiredTxs() + if xp.idTab.len >= MAX_POOL_SIZE: return err(txErrorPoolIsFull) @@ -287,30 +313,8 @@ proc addTx*(xp: TxPoolRef, ptx: PooledTransaction): Result[void, TxError] = proc addTx*(xp: TxPoolRef, tx: Transaction): Result[void, TxError] = xp.addTx(PooledTransaction(tx: tx)) -proc getItem*(xp: TxPoolRef, id: Hash32): Result[TxItemRef, TxError] = - let item = xp.idTab.getOrDefault(id) - if item.isNil: - return err(txErrorItemNotFound) - ok(item) - -proc removeTx*(xp: TxPoolRef, id: Hash32) = - let item = xp.getItem(id).valueOr: - return - xp.removeFromSenderTab(item) - xp.idTab.del(id) - -proc removeExpiredTxs*(xp: TxPoolRef, lifeTime: Duration = TX_ITEM_LIFETIME) = - var expired = newSeqOfCap[Hash32](xp.idTab.len div 4) - let now = utcNow() - - for txHash, item in xp.idTab: - if now - item.time > lifeTime: - expired.add txHash - - for txHash in expired: - xp.removeTx(txHash) iterator byPriceAndNonce*(xp: TxPoolRef): TxItemRef = - for item in byPriceAndNonce(xp.senderTab, + for item in byPriceAndNonce(xp.senderTab, xp.idTab, xp.vmState.ledger, xp.baseFee): yield item diff --git a/nimbus/core/tx_pool/tx_tabs.nim b/nimbus/core/tx_pool/tx_tabs.nim index 049b29033b..d78e7cefe1 100644 --- a/nimbus/core/tx_pool/tx_tabs.nim +++ b/nimbus/core/tx_pool/tx_tabs.nim @@ -45,6 +45,7 @@ func len*(sn: TxSenderNonceRef): auto = sn.list.len iterator byPriceAndNonce*(senderTab: TxSenderTab, + idTab: var TxIdTab, ledger: LedgerRef, baseFee: GasInt): TxItemRef = template removeFirstAndPushTo(sn, byPrice) = @@ -54,25 +55,32 @@ iterator byPriceAndNonce*(senderTab: TxSenderTab, byPrice.push(rc.data) var byNonce: TxSenderTab - for address, sn in senderTab: - # Check if the account nonce matches the lowest known tx nonce + for address, sn in senderTab: var - nonce = ledger.getNonce(address) - rc = sn.list.eq(nonce) + nonce = ledger.getNonce(address) sortedByNonce: TxSenderNonceRef - + + # Remove item with nonce lower than current account. + # Happen when proposed block rejected. + var rc = sn.list.lt(nonce) + while rc.isOk: + let item = rc.get.data + idTab.del(item.id) + discard sn.list.delete(item.nonce) + rc = sn.list.lt(nonce) + + # Check if the account nonce matches the lowest known tx nonce + rc = sn.list.ge(nonce) while rc.isOk: let item = rc.get.data item.calculatePrice(baseFee) - if item.nonce != nonce: - # a gap in nonce, stop collecting - break - + if sortedByNonce.isNil: sortedByNonce = TxSenderNonceRef.init() byNonce[address] = sortedByNonce sortedByNonce.insertOrReplace(item) + # If there is a gap, sn.list.eq will return isErr nonce = item.nonce + 1 rc = sn.list.eq(nonce) diff --git a/nimbus/core/validate.nim b/nimbus/core/validate.nim index 9fddf64b54..acf715e5a7 100644 --- a/nimbus/core/validate.nim +++ b/nimbus/core/validate.nim @@ -228,7 +228,7 @@ proc validateTxBasic*( # The total must be the larger of the two if tx.maxFeePerGasNorm < tx.maxPriorityFeePerGasNorm: - return err(&"invalid tx: maxFee is smaller than maPriorityFee. maxFee={tx.maxFeePerGas}, maxPriorityFee={tx.maxPriorityFeePerGasNorm}") + return err(&"invalid tx: maxFee is smaller than maxPriorityFee. maxFee={tx.maxFeePerGas}, maxPriorityFee={tx.maxPriorityFeePerGasNorm}") if tx.gasLimit < tx.intrinsicGas(fork): return err(&"invalid tx: not enough gas to perform calculation. avail={tx.gasLimit}, require={tx.intrinsicGas(fork)}") diff --git a/tests/test_txpool.nim b/tests/test_txpool.nim index fcaea639a6..dad8b64b68 100644 --- a/tests/test_txpool.nim +++ b/tests/test_txpool.nim @@ -27,6 +27,7 @@ import const genesisFile = "tests/customgenesis/merge.json" feeRecipient = address"0000000000000000000000000000000000000212" + recipient = address"0000000000000000000000000000000000000213" prevRandao = Bytes32 EMPTY_UNCLE_HASH type @@ -60,7 +61,7 @@ proc initEnv(envFork: HardFork): TestEnv = let # create the sender first, because it will modify networkParams - sender = TxSender.new(conf.networkParams) + sender = TxSender.new(conf.networkParams, 30) com = CommonRef.new(newCoreDbRef DefaultDbMemory, nil, conf.networkId, conf.networkParams) chain = ForkedChainRef.init(com) @@ -73,6 +74,61 @@ proc initEnv(envFork: HardFork): TestEnv = sender: sender ) +template checkAddTx(xp, tx, errorCode) = + let prevCount = xp.len + let rc = xp.addTx(tx) + check rc.isErr == true + if rc.isErr: + check rc.error == errorCode + check xp.len == prevCount + +template checkAddTx(xp, tx) = + let expCount = xp.len + 1 + let rc = xp.addTx(tx) + check rc.isOk == true + if rc.isErr: + debugEcho "ADD TX: ", rc.error + check xp.len == expCount + +template checkAddTxSupersede(xp, tx) = + let prevCount = xp.len + let rc = xp.addTx(tx) + check rc.isOk == true + if rc.isErr: + debugEcho "ADD TX SUPERSEDE: ", rc.error + check xp.len == prevCount + +template checkAssembleBlock(xp, expCount): auto = + xp.com.pos.timestamp = xp.com.pos.timestamp + 1 + let rc = xp.assembleBlock() + check rc.isOk == true + if rc.isErr: + debugEcho "ASSEMBLE BLOCK: ", rc.error + if rc.isOk: + check rc.value.blk.transactions.len == expCount + rc.get + +template checkImportBlock(xp: TxPoolRef, bundle: AssembledBlock) = + let rc = xp.chain.importBlock(bundle.blk) + check rc.isOk == true + if rc.isErr: + debugEcho "IMPORT BLOCK: ", rc.error + +template checkImportBlock(xp: TxPoolRef, expCount: int, expRem: int) = + let bundle = checkAssembleBlock(xp, expCount) + checkImportBlock(xp, bundle) + xp.removeNewBlockTxs(bundle.blk) + check xp.len == expRem + +template checkImportBlock(xp: TxPoolRef, expCount: int) = + let bundle = checkAssembleBlock(xp, expCount) + checkImportBlock(xp, bundle) + +template checkImportBlock(xp: TxPoolRef, bundle: AssembledBlock, expRem: int) = + checkImportBlock(xp, bundle) + xp.removeNewBlockTxs(bundle.blk) + check xp.len == expRem + proc txPoolMain*() = suite "TxPool test suite": loadKzgTrustedSetup().expect("KZG trusted setup loaded") @@ -83,6 +139,8 @@ proc txPoolMain*() = chain = env.chain com = env.com + com.pos.prevRandao = prevRandao + com.pos.feeRecipient = feeRecipient com.pos.timestamp = EthTime.now() test "Bad blob tx": @@ -97,10 +155,7 @@ proc txPoolMain*() = var z = ptx.networkPayload.blobs[0] z[0] = not z[0] ptx.networkPayload.blobs[0] = z - let rc = xp.addTx(ptx) - check rc.isErr - check rc.error == txErrorInvalidBlob - check xp.len == 0 + xp.checkAddTx(ptx, txErrorInvalidBlob) test "Bad chainId": let acc = mx.getAccount(1) @@ -111,77 +166,32 @@ proc txPoolMain*() = let ccid = ptx.tx.chainId.uint64 let cid = Opt.some(ChainId(ccid.not)) ptx.tx = mx.customizeTransaction(acc, ptx.tx, CustomTx(chainId: cid)) - let rc = xp.addTx(ptx) - check rc.isErr - check rc.error == txErrorChainIdMismatch - check xp.len == 0 + xp.checkAddTx(ptx, txErrorChainIdMismatch) test "Basic validation error, gas limit too low": let tc = BaseTx( gasLimit: 18000 ) - var ptx = mx.makeTx(tc, 0) - let rc = xp.addTx(ptx) - check rc.isErr - check rc.error == txErrorBasicValidation - check xp.len == 0 + let ptx = mx.makeTx(tc, 0) + xp.checkAddTx(ptx, txErrorBasicValidation) test "Known tx": let tc = BaseTx( gasLimit: 75000 ) let ptx = mx.makeNextTx(tc) - check xp.addTx(ptx).isOk - let rc = xp.addTx(ptx) - check rc.isErr - check rc.error == txErrorAlreadyKnown - check xp.len == 1 - - com.pos.prevRandao = prevRandao - com.pos.feeRecipient = feeRecipient - com.pos.timestamp = com.pos.timestamp + 1 - let bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 1 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return - xp.removeNewBlockTxs(bundle.blk) - check xp.len == 0 + xp.checkAddTx(ptx) + xp.checkAddTx(ptx, txErrorAlreadyKnown) + xp.checkImportBlock(1, 0) test "nonce too small": let tc = BaseTx( gasLimit: 75000 ) let ptx = mx.makeNextTx(tc) - check xp.addTx(ptx).isOk - check xp.len == 1 - - com.pos.prevRandao = prevRandao - com.pos.feeRecipient = feeRecipient - com.pos.timestamp = com.pos.timestamp + 1 - let bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 1 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return - - xp.removeNewBlockTxs(bundle.blk) - check xp.len == 0 - - let rc = xp.addTx(ptx) - check rc.isErr - check rc.error == txErrorNonceTooSmall - check xp.len == 0 + xp.checkAddTx(ptx) + xp.checkImportBlock(1, 0) + xp.checkAddTx(ptx, txErrorNonceTooSmall) test "nonce gap after account nonce": let acc = mx.getAccount(13) @@ -189,41 +199,14 @@ proc txPoolMain*() = gasLimit: 75000 ) let ptx1 = mx.makeTx(tc, acc, 1) - check xp.addTx(ptx1).isOk - check xp.len == 1 - - com.pos.prevRandao = prevRandao - com.pos.feeRecipient = feeRecipient - com.pos.timestamp = com.pos.timestamp + 1 - var bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 0 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return + xp.checkAddTx(ptx1) + + xp.checkImportBlock(0) let ptx0 = mx.makeTx(tc, acc, 0) - check xp.addTx(ptx0).isOk - check xp.len == 2 - - com.pos.timestamp = com.pos.timestamp + 1 - bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 2 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return - - xp.removeNewBlockTxs(bundle.blk) - check xp.len == 0 + xp.checkAddTx(ptx0) + + xp.checkImportBlock(2, 0) test "nonce gap in the middle of nonces": let acc = mx.getAccount(14) @@ -232,84 +215,36 @@ proc txPoolMain*() = ) let ptx0 = mx.makeTx(tc, acc, 0) - check xp.addTx(ptx0).isOk - check xp.len == 1 + xp.checkAddTx(ptx0) let ptx2 = mx.makeTx(tc, acc, 2) - check xp.addTx(ptx2).isOk - check xp.len == 2 - - com.pos.prevRandao = prevRandao - com.pos.feeRecipient = feeRecipient - com.pos.timestamp = com.pos.timestamp + 1 - var bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 1 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return - - xp.removeNewBlockTxs(bundle.blk) - check xp.len == 1 + xp.checkAddTx(ptx2) + + xp.checkImportBlock(1, 1) let ptx1 = mx.makeTx(tc, acc, 1) - check xp.addTx(ptx1).isOk - check xp.len == 2 - - com.pos.timestamp = com.pos.timestamp + 1 - bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 2 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return - - xp.removeNewBlockTxs(bundle.blk) - check xp.len == 0 + xp.checkAddTx(ptx1) + + xp.checkImportBlock(2, 0) test "supersede existing tx": let acc = mx.getAccount(15) let tc = BaseTx( + txType: Opt.some(TxLegacy), gasLimit: 75000 ) var ptx = mx.makeTx(tc, acc, 0) - check xp.addTx(ptx).isOk - check xp.len == 1 + xp.checkAddTx(ptx) let oldPrice = ptx.tx.gasPrice ptx.tx = mx.customizeTransaction(acc, ptx.tx, CustomTx(gasPriceOrGasFeeCap: Opt.some(oldPrice*2))) - check xp.addTx(ptx).isOk - check xp.len == 1 - - com.pos.prevRandao = prevRandao - com.pos.feeRecipient = feeRecipient - com.pos.timestamp = com.pos.timestamp + 1 - var bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 1 - check bundle.blk.transactions[0].gasPrice == oldPrice*2 - - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return - - xp.removeNewBlockTxs(bundle.blk) - check xp.len == 0 + xp.checkAddTxSupersede(ptx) + let bundle = xp.checkAssembleBlock(1) + check bundle.blk.transactions[0].gasPrice == oldPrice*2 + xp.checkImportBlock(bundle, 0) test "removeNewBlockTxs after two blocks": let tc = BaseTx( @@ -317,77 +252,38 @@ proc txPoolMain*() = ) var ptx = mx.makeNextTx(tc) - check xp.addTx(ptx).isOk - check xp.len == 1 - - com.pos.prevRandao = prevRandao - com.pos.feeRecipient = feeRecipient - com.pos.timestamp = com.pos.timestamp + 1 - var bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 1 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return + xp.checkAddTx(ptx) + + xp.checkImportBlock(1) ptx = mx.makeNextTx(tc) - check xp.addTx(ptx).isOk - check xp.len == 2 - - com.pos.timestamp = com.pos.timestamp + 1 - bundle = xp.assembleBlock().valueOr: - debugEcho error - check false - return - - check bundle.blk.transactions.len == 1 - chain.importBlock(bundle.blk).isOkOr: - check false - debugEcho error - return - - xp.removeNewBlockTxs(bundle.blk) - check xp.len == 0 + xp.checkAddTx(ptx) + + xp.checkImportBlock(1, 0) test "max transactions per account": let acc = mx.getAccount(16) let tc = BaseTx( + txType: Opt.some(TxLegacy), gasLimit: 75000 ) const MAX_TXS_GENERATED = 100 for i in 0..MAX_TXS_GENERATED-2: let ptx = mx.makeTx(tc, acc, i.AccountNonce) - check xp.addTx(ptx).isOk - check xp.len == i + 1 + xp.checkAddTx(ptx) var ptx = mx.makeTx(tc, acc, MAX_TXS_GENERATED-1) - check xp.addTx(ptx).isOk - check xp.len == MAX_TXS_GENERATED + xp.checkAddTx(ptx) - var ptxMax = mx.makeTx(tc, acc, MAX_TXS_GENERATED) - let rc = xp.addTx(ptxMax) - check rc.isErr - check rc.error == txErrorSenderMaxTxs - check xp.len == MAX_TXS_GENERATED + let ptxMax = mx.makeTx(tc, acc, MAX_TXS_GENERATED) + xp.checkAddTx(ptxMax, txErrorSenderMaxTxs) # superseding not hit sender max txs let oldPrice = ptx.tx.gasPrice ptx.tx = mx.customizeTransaction(acc, ptx.tx, CustomTx(gasPriceOrGasFeeCap: Opt.some(oldPrice*2))) - xp.addTx(ptx).isOkOr: - debugEcho error - check false - return - - check xp.len == MAX_TXS_GENERATED - - com.pos.prevRandao = prevRandao - com.pos.feeRecipient = feeRecipient + xp.checkAddTxSupersede(ptx) var numTxsPacked = 0 while numTxsPacked < MAX_TXS_GENERATED: @@ -408,5 +304,52 @@ proc txPoolMain*() = check xp.len == 0 + test "auto remove lower nonces": + let xp2 = TxPoolRef.new(chain) + let acc = mx.getAccount(17) + let tc = BaseTx( + gasLimit: 75000 + ) + + let ptx0 = mx.makeTx(tc, acc, 0) + xp.checkAddTx(ptx0) + xp2.checkAddTx(ptx0) + + let ptx1 = mx.makeTx(tc, acc, 1) + xp.checkAddTx(ptx1) + xp2.checkAddTx(ptx1) + + let ptx2 = mx.makeTx(tc, acc, 2) + xp.checkAddTx(ptx2) + + xp2.checkImportBlock(2, 0) + xp.checkImportBlock(1, 0) + + test "mixed type of transactions": + let acc = mx.getAccount(18) + let + tc1 = BaseTx( + txType: Opt.some(TxLegacy), + gasLimit: 75000 + ) + tc2 = BaseTx( + txType: Opt.some(TxEip1559), + gasLimit: 75000 + ) + tc3 = BaseTx( + txType: Opt.some(TxEip4844), + recipient: Opt.some(recipient), + gasLimit: 75000 + ) + + let ptx0 = mx.makeTx(tc1, acc, 0) + let ptx1 = mx.makeTx(tc2, acc, 1) + let ptx2 = mx.makeTx(tc3, acc, 2) + + xp.checkAddTx(ptx0) + xp.checkAddTx(ptx1) + xp.checkAddTx(ptx2) + xp.checkImportBlock(3, 0) + when isMainModule: txPoolMain()