From d5b644fd3365741cc07aa23ea1a6ee132657946a Mon Sep 17 00:00:00 2001 From: Dima Litvinov Date: Fri, 27 Oct 2023 19:51:09 +0100 Subject: [PATCH 1/4] IS-879 Re-read cached receipt from disk if gasUsed = 0 --- libethereum/BlockChain.h | 40 +++++++++++++++++++++--- test/unittests/libethereum/SkaleHost.cpp | 38 +++++++++++++++++----- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/libethereum/BlockChain.h b/libethereum/BlockChain.h index cd12f7e5d..f07a3dbc5 100644 --- a/libethereum/BlockChain.h +++ b/libethereum/BlockChain.h @@ -217,12 +217,13 @@ class BlockChain { /// Get the transaction receipt by transaction hash. Thread-safe. TransactionReceipt transactionReceipt( h256 const& _transactionHash ) const { - TransactionAddress ta = - queryExtras< TransactionAddress, ExtraTransactionAddress >( _transactionHash, - m_transactionAddresses, x_transactionAddresses, NullTransactionAddress ); - if ( !ta ) + std::pair< h256, unsigned > tl = transactionLocation( _transactionHash ); + + if ( !tl.first ) return bytesConstRef(); - return transactionReceipt( ta.blockHash, ta.index ); + + // guaranteed to be cached by transactionLocation() call above + return transactionReceipt( tl.first, tl.second ); } /// Get a list of transaction hashes for a given block. Thread-safe. @@ -312,11 +313,40 @@ class BlockChain { return transaction( ta.blockHash, ta.index ); } std::pair< h256, unsigned > transactionLocation( h256 const& _transactionHash ) const { + // cached transactionAddresses for transactions with gasUsed==0 should be re-queried from DB + bool cached = false; + { + ReadGuard g( x_transactionAddresses ); + cached = m_transactionAddresses.count( _transactionHash ) > 0; + } + + // get transactionAddresses from DB or cache TransactionAddress ta = queryExtras< TransactionAddress, ExtraTransactionAddress >( _transactionHash, m_transactionAddresses, x_transactionAddresses, NullTransactionAddress ); + if ( !ta ) return std::pair< h256, unsigned >( h256(), 0 ); + + // compute gas used + TransactionReceipt receipt = transactionReceipt( ta.blockHash, ta.index ); + u256 cumulativeGasUsed = receipt.cumulativeGasUsed(); + u256 prevGasUsed = ta.index == 0 ? + 0 : + transactionReceipt( ta.blockHash, ta.index - 1 ).cumulativeGasUsed(); + u256 gasUsed = cumulativeGasUsed - prevGasUsed; + + // re-query receipt from DB if gasUsed==0 (and cache might have wrong value) + if ( gasUsed == 0 && cached ) { + // remove from cache + { + WriteGuard g( x_transactionAddresses ); + m_transactionAddresses.erase( _transactionHash ); + } + // re-read from DB + ta = queryExtras< TransactionAddress, ExtraTransactionAddress >( _transactionHash, + m_transactionAddresses, x_transactionAddresses, NullTransactionAddress ); + } return std::make_pair( ta.blockHash, ta.index ); } diff --git a/test/unittests/libethereum/SkaleHost.cpp b/test/unittests/libethereum/SkaleHost.cpp index 07b744a50..fc617fc23 100644 --- a/test/unittests/libethereum/SkaleHost.cpp +++ b/test/unittests/libethereum/SkaleHost.cpp @@ -194,8 +194,8 @@ struct SkaleHostFixture : public TestOutputHelperFixture { #define CHECK_BLOCK_BEGIN auto blockBefore = client->number() #define REQUIRE_BLOCK_INCREASE( increase ) \ - auto blockAfter = client->number(); \ - BOOST_REQUIRE_EQUAL( blockAfter - blockBefore, increase ) + { auto blockAfter = client->number(); \ + BOOST_REQUIRE_EQUAL( blockAfter - blockBefore, increase ); } #define REQUIRE_BLOCK_SIZE( number, s ) \ { \ @@ -214,18 +214,18 @@ struct SkaleHostFixture : public TestOutputHelperFixture { #define CHECK_NONCE_BEGIN( senderAddress ) u256 nonceBefore = client->countAt( senderAddress ) #define REQUIRE_NONCE_INCREASE( senderAddress, increase ) \ - u256 nonceAfter = client->countAt( senderAddress ); \ - BOOST_REQUIRE_EQUAL( nonceAfter - nonceBefore, increase ) + { u256 nonceAfter = client->countAt( senderAddress ); \ + BOOST_REQUIRE_EQUAL( nonceAfter - nonceBefore, increase ); } #define CHECK_BALANCE_BEGIN( senderAddress ) u256 balanceBefore = client->balanceAt( senderAddress ) #define REQUIRE_BALANCE_DECREASE( senderAddress, decrease ) \ - u256 balanceAfter = client->balanceAt( senderAddress ); \ - BOOST_REQUIRE_EQUAL( balanceBefore - balanceAfter, decrease ) + { u256 balanceAfter = client->balanceAt( senderAddress ); \ + BOOST_REQUIRE_EQUAL( balanceBefore - balanceAfter, decrease ); } #define REQUIRE_BALANCE_DECREASE_GE( senderAddress, decrease ) \ - u256 balanceAfter = client->balanceAt( senderAddress ); \ - BOOST_REQUIRE_GE( balanceBefore - balanceAfter, decrease ) + { u256 balanceAfter = client->balanceAt( senderAddress ); \ + BOOST_REQUIRE_GE( balanceBefore - balanceAfter, decrease ); } BOOST_FIXTURE_TEST_SUITE( SkaleHostSuite, SkaleHostFixture ) //, *boost::unit_test::disabled() ) @@ -752,6 +752,28 @@ BOOST_DATA_TEST_CASE( transactionBalanceBad, skipInvalidTransactionsVariants, sk REQUIRE_NONCE_INCREASE( senderAddress, 0 ); REQUIRE_BALANCE_DECREASE( senderAddress, 0 ); + + // step 2: check receipt + + if(!skipInvalidTransactionsFlag){ + LocalisedTransactionReceipt r1 = client->localisedTransactionReceipt(txHash); + BOOST_REQUIRE_EQUAL(r1.blockNumber(), 1); + BOOST_REQUIRE_EQUAL(r1.gasUsed(), 0); + } + + // make money + dev::eth::simulateMining( *client, 1, senderAddress ); + + stub->createBlock( ConsensusExtFace::transactions_vector{stream.out()}, utcTime(), 2U ); + + REQUIRE_BLOCK_SIZE( 2, 1 ); + REQUIRE_BLOCK_TRANSACTION( 2, 0, txHash ); + REQUIRE_NONCE_INCREASE( senderAddress, 1 ); + REQUIRE_BALANCE_DECREASE_GE( senderAddress, 1 ); + + LocalisedTransactionReceipt r2 = client->localisedTransactionReceipt(txHash); + BOOST_REQUIRE_EQUAL(r2.blockNumber(), 2); + BOOST_REQUIRE_GE(r2.gasUsed(), 21000); } // Transaction should be IGNORED during execution or absent if skipInvalidTransactionsFlag From 48ad7c3607d995e097e31aa4aae51e18cafb366b Mon Sep 17 00:00:00 2001 From: Dima Litvinov Date: Mon, 30 Oct 2023 17:20:58 +0000 Subject: [PATCH 2/4] IS-879 More testing --- libethereum/BlockChain.h | 2 +- test/unittests/libethereum/SkaleHost.cpp | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/libethereum/BlockChain.h b/libethereum/BlockChain.h index f07a3dbc5..16bf59b1e 100644 --- a/libethereum/BlockChain.h +++ b/libethereum/BlockChain.h @@ -222,7 +222,7 @@ class BlockChain { if ( !tl.first ) return bytesConstRef(); - // guaranteed to be cached by transactionLocation() call above + // in most cases will hit cache because of transactionLocation() call above return transactionReceipt( tl.first, tl.second ); } diff --git a/test/unittests/libethereum/SkaleHost.cpp b/test/unittests/libethereum/SkaleHost.cpp index fc617fc23..1e5590e4a 100644 --- a/test/unittests/libethereum/SkaleHost.cpp +++ b/test/unittests/libethereum/SkaleHost.cpp @@ -753,12 +753,14 @@ BOOST_DATA_TEST_CASE( transactionBalanceBad, skipInvalidTransactionsVariants, sk REQUIRE_NONCE_INCREASE( senderAddress, 0 ); REQUIRE_BALANCE_DECREASE( senderAddress, 0 ); - // step 2: check receipt + // step 2: check that receipt "moved" to another block after successfull re-execution of the same transaction if(!skipInvalidTransactionsFlag){ LocalisedTransactionReceipt r1 = client->localisedTransactionReceipt(txHash); BOOST_REQUIRE_EQUAL(r1.blockNumber(), 1); BOOST_REQUIRE_EQUAL(r1.gasUsed(), 0); + LocalisedTransaction lt = client->localisedTransaction(txHash); + BOOST_REQUIRE_EQUAL(lt.blockNumber(), 1); } // make money @@ -774,6 +776,8 @@ BOOST_DATA_TEST_CASE( transactionBalanceBad, skipInvalidTransactionsVariants, sk LocalisedTransactionReceipt r2 = client->localisedTransactionReceipt(txHash); BOOST_REQUIRE_EQUAL(r2.blockNumber(), 2); BOOST_REQUIRE_GE(r2.gasUsed(), 21000); + LocalisedTransaction lt = client->localisedTransaction(txHash); + BOOST_REQUIRE_EQUAL(lt.blockNumber(), 2); } // Transaction should be IGNORED during execution or absent if skipInvalidTransactionsFlag From eab517b25c919414c497b0791d28eba00a80b3d8 Mon Sep 17 00:00:00 2001 From: Dima Litvinov Date: Mon, 30 Oct 2023 19:56:20 +0000 Subject: [PATCH 3/4] IS-879 More tests --- test/unittests/libweb3jsonrpc/jsonrpc.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/unittests/libweb3jsonrpc/jsonrpc.cpp b/test/unittests/libweb3jsonrpc/jsonrpc.cpp index 5ef568135..1da3f94a9 100644 --- a/test/unittests/libweb3jsonrpc/jsonrpc.cpp +++ b/test/unittests/libweb3jsonrpc/jsonrpc.cpp @@ -2988,6 +2988,20 @@ BOOST_AUTO_TEST_CASE( skip_invalid_transactions ) { BOOST_REQUIRE_EQUAL(cnt.asString(), "0x3"); cnt = fixture.rpcClient->eth_getBlockTransactionCountByHash(bh); BOOST_REQUIRE_EQUAL(cnt.asString(), "0x3"); + + // send it successfully + + // make money + dev::eth::simulateMining( *fixture.client, 1); + + h2 = fixture.client->importTransaction( tx2 ); // invalid + + dev::eth::mineTransaction(*(fixture.client), 1); + + // checks: + Json::Value r2; + BOOST_REQUIRE_NO_THROW(r2 = fixture.rpcClient->eth_getTransactionReceipt(toJS(h2))); + BOOST_REQUIRE_EQUAL(r2["blockNumber"], toJS(fixture.client->number())); #endif } From 7ba33e33ed7f5eb2a5aada36ac74650517edc6b4 Mon Sep 17 00:00:00 2001 From: Dima Litvinov Date: Thu, 2 Nov 2023 13:00:21 +0000 Subject: [PATCH 4/4] IS-879 Remove unneeded checks after patch timestamp --- libethereum/BlockChain.cpp | 62 ++++++++++++++++++++++++++++++++++++++ libethereum/BlockChain.h | 37 +---------------------- 2 files changed, 63 insertions(+), 36 deletions(-) diff --git a/libethereum/BlockChain.cpp b/libethereum/BlockChain.cpp index fcca55c02..677d19b48 100644 --- a/libethereum/BlockChain.cpp +++ b/libethereum/BlockChain.cpp @@ -44,6 +44,7 @@ #include #include +#include #include #include "Block.h" @@ -172,6 +173,24 @@ unsigned c_maxCacheSize = 1024 * 1024 * 64; /// Min size, below which we don't bother flushing it. unsigned c_minCacheSize = 1024 * 1024 * 32; +bool hasPotentialInvalidTransactionsInBlock( BlockNumber _bn, const BlockChain& _bc ) { + if ( _bn == 0 ) + return false; + + if ( SkipInvalidTransactionsPatch::getActivationTimestamp() == 0 ) + return true; + + if ( _bn == PendingBlock ) + return !SkipInvalidTransactionsPatch::isEnabled(); + + if ( _bn == LatestBlock ) + _bn = _bc.number(); + + time_t prev_ts = _bc.info( _bc.numberHash( _bn - 1 ) ).timestamp(); + + return prev_ts < SkipInvalidTransactionsPatch::getActivationTimestamp(); +} + string BlockChain::getChainDirName( const ChainParams& _cp ) { return toHex( BlockHeader( _cp.genesisBlock() ).hash().ref().cropped( 0, 4 ) ); } @@ -334,6 +353,49 @@ void BlockChain::close() { m_lastBlockHashes->clear(); } +std::pair< h256, unsigned > BlockChain::transactionLocation( h256 const& _transactionHash ) const { + // cached transactionAddresses for transactions with gasUsed==0 should be re-queried from DB + bool cached = false; + { + ReadGuard g( x_transactionAddresses ); + cached = m_transactionAddresses.count( _transactionHash ) > 0; + } + + // get transactionAddresses from DB or cache + TransactionAddress ta = queryExtras< TransactionAddress, ExtraTransactionAddress >( + _transactionHash, m_transactionAddresses, x_transactionAddresses, NullTransactionAddress ); + + if ( !ta ) + return std::pair< h256, unsigned >( h256(), 0 ); + + auto blockNumber = this->number( ta.blockHash ); + + if ( !hasPotentialInvalidTransactionsInBlock( blockNumber, *this ) ) + return std::make_pair( ta.blockHash, ta.index ); + + // rest is for blocks with possibility of invalid transactions + + // compute gas used + TransactionReceipt receipt = transactionReceipt( ta.blockHash, ta.index ); + u256 cumulativeGasUsed = receipt.cumulativeGasUsed(); + u256 prevGasUsed = + ta.index == 0 ? 0 : transactionReceipt( ta.blockHash, ta.index - 1 ).cumulativeGasUsed(); + u256 gasUsed = cumulativeGasUsed - prevGasUsed; + + // re-query receipt from DB if gasUsed==0 (and cache might have wrong value) + if ( gasUsed == 0 && cached ) { + // remove from cache + { + WriteGuard g( x_transactionAddresses ); + m_transactionAddresses.erase( _transactionHash ); + } + // re-read from DB + ta = queryExtras< TransactionAddress, ExtraTransactionAddress >( _transactionHash, + m_transactionAddresses, x_transactionAddresses, NullTransactionAddress ); + } + return std::make_pair( ta.blockHash, ta.index ); +} + string BlockChain::dumpDatabase() const { ostringstream oss; oss << m_lastBlockHash << '\n'; diff --git a/libethereum/BlockChain.h b/libethereum/BlockChain.h index 16bf59b1e..593475fb1 100644 --- a/libethereum/BlockChain.h +++ b/libethereum/BlockChain.h @@ -312,43 +312,8 @@ class BlockChain { return bytes(); return transaction( ta.blockHash, ta.index ); } - std::pair< h256, unsigned > transactionLocation( h256 const& _transactionHash ) const { - // cached transactionAddresses for transactions with gasUsed==0 should be re-queried from DB - bool cached = false; - { - ReadGuard g( x_transactionAddresses ); - cached = m_transactionAddresses.count( _transactionHash ) > 0; - } - - // get transactionAddresses from DB or cache - TransactionAddress ta = - queryExtras< TransactionAddress, ExtraTransactionAddress >( _transactionHash, - m_transactionAddresses, x_transactionAddresses, NullTransactionAddress ); - if ( !ta ) - return std::pair< h256, unsigned >( h256(), 0 ); - - // compute gas used - TransactionReceipt receipt = transactionReceipt( ta.blockHash, ta.index ); - u256 cumulativeGasUsed = receipt.cumulativeGasUsed(); - u256 prevGasUsed = ta.index == 0 ? - 0 : - transactionReceipt( ta.blockHash, ta.index - 1 ).cumulativeGasUsed(); - u256 gasUsed = cumulativeGasUsed - prevGasUsed; - - // re-query receipt from DB if gasUsed==0 (and cache might have wrong value) - if ( gasUsed == 0 && cached ) { - // remove from cache - { - WriteGuard g( x_transactionAddresses ); - m_transactionAddresses.erase( _transactionHash ); - } - // re-read from DB - ta = queryExtras< TransactionAddress, ExtraTransactionAddress >( _transactionHash, - m_transactionAddresses, x_transactionAddresses, NullTransactionAddress ); - } - return std::make_pair( ta.blockHash, ta.index ); - } + std::pair< h256, unsigned > transactionLocation( h256 const& _transactionHash ) const; /// Get a block's transaction (RLP format) for the given block hash (or the most recent mined if /// none given) & index. Thread-safe.