Fix/6790 return accepted for rejected tx added to broadcast #6889

pxyxyrus · 2024-04-02T00:46:44Z

Fixes Closes Resolves #

Issue 6790
#6790

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Notes on testing

The change broke several existing tests, so the changes made in the test code were to make the test pass.
However, reviews are needed.

Demuirgos · 2024-04-02T01:05:59Z

src/Nethermind/Nethermind.TxPool/TxPool.cs

                {
-                    // we are adding only to persistent broadcast - not good enough for standard pool,
-                    // but can be good enough for TxBroadcaster pool - for local txs only
-                    _broadcaster.Broadcast(tx, isPersistentBroadcast);


what happened to _broadcaster.Broadcast call?

Since the if branch now has the opposite condition, if the tx is persistent (and not a blob tx), it would run to the end of the function (line 462~483).

_broadcaster.Broadcast will be run at line 477.

Not sure if running through all the normal flow here is best choice as this is a special case, when something we are not adding to main pool will have broadcasting, will wait for @marcindsobczak opinion.

Not a good option, we definitely don't want to run through all the normal flow here

marcindsobczak

There are a few things which should be done before returning Accepted:

check if tx was successfully added to broadcaster collection - I recommend to modify Broadcast to TryBroadcast and return true if successfully added
check if tx is not blob type (we shouldn't add blob tx to broadcaster if we didn't accepted it to standard blob pool - because we are not keeping actual blobs in broadcaster)

Optionally (not must have, but good to have) write a test trying to add different types of txs to already full tx pool and not full persistent pool (so basically testing adding txs scenario with rejecting from standard pool and adding to broadcaster collection) - it should reject blob tx and accept all the other types

marcindsobczak · 2024-04-04T08:33:57Z

src/Nethermind/Nethermind.TxPool/TxPool.cs

                {
-                    // we are adding only to persistent broadcast - not good enough for standard pool,
-                    // but can be good enough for TxBroadcaster pool - for local txs only
-                    _broadcaster.Broadcast(tx, isPersistentBroadcast);


Not a good option, we definitely don't want to run through all the normal flow here

marcindsobczak · 2024-04-04T08:35:47Z

src/Nethermind/Nethermind.TxPool/TxPool.cs

+                if (isPersistentBroadcast) {
                    // we are adding only to persistent broadcast - not good enough for standard pool,
                    // but can be good enough for TxBroadcaster pool - for local txs only
                    _broadcaster.Broadcast(tx, isPersistentBroadcast);
+                    return AcceptTxResult.Accepted;
+                } else {
+                    Metrics.PendingTransactionsPassedFiltersButCannotCompeteOnFees++;
+                    return AcceptTxResult.FeeTooLowToCompete;
                }


Why accepted? We called _broadcaster.Broadcast(tx, isPersistentBroadcast), but we don't know if tx was added or rejected there. If broadcaster collection is full and tx is worse than the worst in broadcaster, it will be declined.
Also, we need to skip blob txs here as they are not stored as full txs in broadcaster (only in standard blob pool), so we will end announcing txs which we don't actually have.
What I will do here is to refactor Broadcast to TryBroadcast and add blob check, e.g.:

Suggested change

if (isPersistentBroadcast) {

// we are adding only to persistent broadcast - not good enough for standard pool,

// but can be good enough for TxBroadcaster pool - for local txs only

_broadcaster.Broadcast(tx, isPersistentBroadcast);

return AcceptTxResult.Accepted;

} else {

Metrics.PendingTransactionsPassedFiltersButCannotCompeteOnFees++;

return AcceptTxResult.FeeTooLowToCompete;

}

// we are trying to add to persistent broadcast - not good enough for standard pool,

// but can be good enough for TxBroadcaster pool - for local txs only.

// We need to skip blob txs here as they are not stored as full txs in broadcaster

if (!isPersistentBroadcast || tx.SupportsBlobs || !_broadcaster.TryBroadcast(tx, true))

{

Metrics.PendingTransactionsPassedFiltersButCannotCompeteOnFees++;

return AcceptTxResult.FeeTooLowToCompete;

}

return AcceptTxResult.Accepted;

marcindsobczak · 2024-04-04T08:39:45Z

src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs

+        [TestCase(true)]
+        [TestCase(false)]
+        public void should_accept_underpaid_txs_if_persistent(bool isPersistentBroadcast) {
+            // copy and past of the code above for my test use.


No need of adding this test, test which you multiplied is testing local transactions and local is just other name for persistentBroadcast so it is already tested in the test above

marcindsobczak · 2024-04-04T08:47:36Z

src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs

@@ -1530,7 +1564,7 @@ public void Should_not_replace_better_txs_by_worse_ones()
                    .SignedAndResolved(_ethereumEcdsa, privateKeyOfAttacker).TestObject;

                AcceptTxResult result = _txPool.SubmitTx(tx, TxHandlingOptions.PersistentBroadcast);
-                result.Should().Be(AcceptTxResult.FeeTooLowToCompete);
+                result.Should().Be(AcceptTxResult.Accepted);


It definitely shouldn't be accepted here, it is a great example of why we need to refactor Broadcast to be TryBroadcast - then it will return false and we will not return AcceptTxResult.Accepted, but AcceptTxResult.FeeTooLowToCompete.
Not accepting txs in this place is a clue of this test, please not modify it - instead modify nethermind code to pass this test

marcindsobczak · 2024-04-04T08:47:42Z

src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs

@@ -1530,7 +1564,7 @@ public void Should_not_replace_better_txs_by_worse_ones()
                    .SignedAndResolved(_ethereumEcdsa, privateKeyOfAttacker).TestObject;

                AcceptTxResult result = _txPool.SubmitTx(tx, TxHandlingOptions.PersistentBroadcast);
-                result.Should().Be(AcceptTxResult.FeeTooLowToCompete);
+                result.Should().Be(AcceptTxResult.Accepted);

                // newly coming txs should evict themselves
                _txPool.GetPendingTransactionsBySender().Keys.Count.Should().Be(txPoolConfig.Size);


And here we have an evidence that txs from above were not actually accepted. So we can't return AcceptTxResult.Accepted in such scenario

marcindsobczak · 2024-04-04T08:51:42Z

src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs

+                _txPool.SubmitTx(tx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.Accepted);

                // newly coming txs should evict themselves
                _txPool.GetPendingTransactionsBySender().Keys.Count.Should().Be(txPoolConfig.Size);


The same story as in test Should_not_replace_better_txs_by_worse_ones()

marcindsobczak · 2024-04-04T09:02:24Z

src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs

-        [TestCase(9, false)]
+        [TestCase(9, true)]
        [TestCase(11, true)]
        public void Should_not_add_underpaid_tx_even_if_lower_nonces_are_expensive(int gasPrice, bool expectedResult)


This is a clue of this test, please don't modify it. It should pass when you refactor Broadcast to be TryBroadcast and return Accepted only if successfully added to broadcaster collection

marcindsobczak · 2024-04-04T09:28:58Z

In TxBroadcaster it can be something like:

        public bool TryBroadcast(Transaction tx, bool isPersistent)
        {
            if (isPersistent)
            {
                return TryStartBroadcast(tx);
            }

            BroadcastOnce(tx);
            return true;
        }

        private bool TryStartBroadcast(Transaction tx)
        {
            if (tx.Hash is null)
            {
                return false;
            }

            bool wasInserted = _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx);

            // broadcast local tx only if MaxFeePerGas is not lower than configurable percent of current base fee
            // (70% by default). Otherwise only add to persistent txs and broadcast when tx will be ready for inclusion
            if (wasInserted && tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree())
            {
                NotifyPeersAboutLocalTx(tx);
            }

            return wasInserted;
        }

current form:

        public void Broadcast(Transaction tx, bool isPersistent)
        {
            if (isPersistent)
            {
                StartBroadcast(tx);
            }
            else
            {
                BroadcastOnce(tx);
            }
        }

        private void StartBroadcast(Transaction tx)
        {
            // broadcast local tx only if MaxFeePerGas is not lower than configurable percent of current base fee
            // (70% by default). Otherwise only add to persistent txs and broadcast when tx will be ready for inclusion
            if (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree())
            {
                NotifyPeersAboutLocalTx(tx);
            }

            if (tx.Hash is not null)
            {
                _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx);
            }
        }

marcindsobczak · 2024-04-04T09:36:23Z

Or add to persistent broadcast only if fee is at least X percent of current base fee (maybe even better? @LukaszRozmej )

        private bool TryStartBroadcast(Transaction tx)
        {
            // add to persistent txs and broadcast local tx only if MaxFeePerGas is not lower than configurable
            // percent of current base fee (70% by default)
            if (tx.Hash is not null
                && _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx)
                && tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree())
            {
                NotifyPeersAboutLocalTx(tx);
                return true;
            }

            return false;
        }

should_add_underpaid_txs_to_full_TxPool_only_if_local(True) should expect AcceptTxResult.Accepted for underpaid persistent transactions that were broadcasted

should_increase_nonce_when_transaction_not_included_in_txPool_but_broadcasted should expect AcceptTxResult.Accepted for underpaid persistent transactions that were broadcasted

pxyxyrus

Even after implementing the suggested solution, the following tests don't pass.

should_add_underpaid_txs_to_full_TxPool_only_if_local(True)

should_increase_nonce_when_transaction_not_included_in_txPool_but_broadcasted

Should_not_add_underpaid_tx_even_if_lower_nonces_are_expensive(9,False)

Should_not_replace_better_txs_by_worse_ones

Should_not_replace_ready_txs_by_nonce_gap_ones

The first two tests were updated to expect Accepted.

I'm being more careful about updating the other three tests.

src/Nethermind/Nethermind.TxPool/TxPool.cs

marcindsobczak · 2024-04-12T14:19:11Z

src/Nethermind/Nethermind.TxPool/TxBroadcaster.cs

+            bool txInserted = _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx);
+
+            if (!txInserted) return false;
+
            // broadcast local tx only if MaxFeePerGas is not lower than configurable percent of current base fee
            // (70% by default). Otherwise only add to persistent txs and broadcast when tx will be ready for inclusion
            if (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree())
            {
                NotifyPeersAboutLocalTx(tx);
+                return true;
            }

-            if (tx.Hash is not null)
-            {
-                _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx);
-            }
+            return false;


After this change, we will return false if tx was inserted, but is not close to base fee. It will be possible, that we return false, but tx will be included in the block in the future. We should decide on one of two options:

just return txInserted at the end of the method. Even if tx is not good enough to send to peers right now, but if we add it to persistent collection, we need to return true

move attempt to insert tx inside if statement checking price, so just return _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx); inside if and return false at the end

It depends if we want to add local tx with fee lower than 70% of baseFee or reject it. I don't have a strong preference here, you can decide. If I would need to make this decision, I will probably go with option 1

ah sorry I reviewed on wrong commit

marcindsobczak · 2024-04-12T14:25:52Z

src/Nethermind/Nethermind.TxPool/TxBroadcaster.cs

@@ -97,31 +97,31 @@ internal class TxBroadcaster : IDisposable
        // only for testing reasons
        internal Transaction[] GetSnapshot() => _persistentTxs.GetSnapshot();

-        public void Broadcast(Transaction tx, bool isPersistent)
+        public bool Broadcast(Transaction tx, bool isPersistent)


Minor cosmetic: would be good to rename method, right now we are not calling it if we want to broadcast (void), but when we want to try to broadcast (bool return depending on success). So I recommend renaming it to TryBroadcast and TryStartBroadcast

Worth to consider before merging

marcindsobczak · 2024-04-12T14:32:57Z

src/Nethermind/Nethermind.TxPool/TxBroadcaster.cs

+        private bool StartBroadcast(Transaction tx)
        {
            // broadcast local tx only if MaxFeePerGas is not lower than configurable percent of current base fee
            // (70% by default). Otherwise only add to persistent txs and broadcast when tx will be ready for inclusion
-            if (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree())
+            if (tx is not null
+                && _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx)
+                && (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree()))
            {
                NotifyPeersAboutLocalTx(tx);
+                return true;
            }

-            if (tx.Hash is not null)
-            {
-                _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx);
-            }
+            return false;
        }


We will reject now to add local txs if fee is lower than X percent (70% as default) of base fee, which is fine

marcindsobczak · 2024-04-12T14:38:59Z

src/Nethermind/Nethermind.TxPool/TxBroadcaster.cs

+            if (tx is not null
+                && _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx)
+                && (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree()))


Ahh but order is wrong, we are firstly trying to insert and then checking fee, so it is possible that we will add tx to persistent collection, but return false. I made this mistake in proposed code in previous review

Suggested change

if (tx is not null

&& _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx)

&& (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree()))

if (tx is not null

&& (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree())

&& _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx))

And there is one more case, thare is a possibility that we will insert tx and evict it just after, so we need logic similar to the one from TxPool. Modify
_persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx)
to be
_persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx, out var removed)
and then check if tx.Hash is the same as removed.Hash. If hashes are the same it means that we didn't actually added tx to persistent collection

It is because we have some capacity, but under the hood we are actually firstly adding new tx (and having capacity max + 1) and then removing the worst one. So if we add and remove the same tx, it means that it wasn't actually added :D

and then tests should work as expected, this is the reason why tests are failing

check if persistentTx added to the local broadcasting pool is immediately removed or not

check fee before inserting tx to the local pool

should_not_broadcast_tx_with_MaxFeePerGas_lower_than_70_percent_of_CurrentBaseFee should not expect tx to be added to local pool when lower than 70% of current base fee

marcindsobczak

Looks good, there are some minor cosmetics which might be improved (I left a few new comments), but generally mergable

marcindsobczak · 2024-04-23T07:57:41Z

src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs

-        [TestCase]
-        public void Should_only_add_legacy_and_1559_local_transactions_to_local_pool_when_underpaid()
+        [TestCase(TxType.Legacy, true)]
+        [TestCase(TxType.EIP1559, true)]


and [TestCase(TxType.AccessList, true)] to have all types

Maybe better to have TestCaseSource and generate test cases for all enum items, by default with true and just have exception for blobs. In that case if we add new tx type we will by default have a test that expects true here.

I've changed the test accordingly to check all the values of TxType.

marcindsobczak · 2024-04-23T07:57:47Z

src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs

        {
-            ISpecProvider specProvider = GetLondonSpecProvider();
-            TxPoolConfig txPoolConfig = new TxPoolConfig { Size = 30 };
+            // Should only add legacy and 1559 local transactions to local pool when underpaid


access list type too and probably all future ones. The best is to write sth like Should only add local transactions other than blob-type to local pool when underpaid

marcindsobczak · 2024-04-23T08:06:01Z

src/Nethermind/Nethermind.TxPool/TxBroadcaster.cs

@@ -97,31 +97,31 @@ internal class TxBroadcaster : IDisposable
        // only for testing reasons
        internal Transaction[] GetSnapshot() => _persistentTxs.GetSnapshot();

-        public void Broadcast(Transaction tx, bool isPersistent)
+        public bool Broadcast(Transaction tx, bool isPersistent)


Worth to consider before merging

…dded_to_broadcast

…for potential tx types

pxyxyrus added 3 commits April 1, 2024 16:39

update tx response for persistent broadcast

ca922d9

added own test

1e4670a

Fix TxPool test cases to expect result according to the changes made

401e318

Demuirgos reviewed Apr 2, 2024

View reviewed changes

Demuirgos requested review from marcindsobczak, benaadams and LukaszRozmej April 2, 2024 03:45

update: persistent tx with low fees broadcasts and returns accepted

13a767a

marcindsobczak requested changes Apr 4, 2024

View reviewed changes

pxyxyrus added 5 commits April 5, 2024 18:04

revert test changes

919bd2d

update broadcaster and underpaid tx broadcast logic

14e8b79

TxBroadcaster code refactor

7936a1e

TxPoolTest update

ca67a97

should_add_underpaid_txs_to_full_TxPool_only_if_local(True) should expect AcceptTxResult.Accepted for underpaid persistent transactions that were broadcasted

TxPoolTest update

6d0a22c

should_increase_nonce_when_transaction_not_included_in_txPool_but_broadcasted should expect AcceptTxResult.Accepted for underpaid persistent transactions that were broadcasted

pxyxyrus commented Apr 11, 2024

View reviewed changes

src/Nethermind/Nethermind.TxPool/TxPool.cs Outdated Show resolved Hide resolved

marcindsobczak reviewed Apr 12, 2024

View reviewed changes

pxyxyrus added 9 commits April 12, 2024 11:58

fix white space formatting

7dfbdb6

fix startBroadcast

4757f6f

check if persistentTx added to the local broadcasting pool is immediately removed or not

fix StartBroadcast

018afc9

check fee before inserting tx to the local pool

TxPoolTest update

be126f5

should_not_broadcast_tx_with_MaxFeePerGas_lower_than_70_percent_of_CurrentBaseFee should not expect tx to be added to local pool when lower than 70% of current base fee

add test to test the updated broadcaster/txpool functionality

b64dd50

test fixed

3c19462

remove invalid import

926c381

remove invalid import

f8546dd

revert commit 926c381

c695cc5

marcindsobczak approved these changes Apr 23, 2024

View reviewed changes

pxyxyrus and others added 4 commits April 23, 2024 14:02

Merge branch 'master' into fix/6790_return_accepted_for_rejected_tx_a…

8360abc

…dded_to_broadcast

add another test case for TxType AccessList

031d15e

change test to check all possible txtype cases and open to extension …

f97394e

…for potential tx types

fix linting error

a6150f1

LukaszRozmej approved these changes May 13, 2024

View reviewed changes

LukaszRozmej merged commit 6a55a67 into NethermindEth:master May 13, 2024
67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/6790 return accepted for rejected tx added to broadcast #6889

Fix/6790 return accepted for rejected tx added to broadcast #6889

pxyxyrus commented Apr 2, 2024

Demuirgos Apr 2, 2024

pxyxyrus Apr 2, 2024

LukaszRozmej Apr 2, 2024 •

edited

marcindsobczak Apr 4, 2024

marcindsobczak left a comment

marcindsobczak Apr 4, 2024

marcindsobczak Apr 4, 2024

marcindsobczak Apr 4, 2024

marcindsobczak Apr 4, 2024

marcindsobczak Apr 4, 2024

marcindsobczak Apr 4, 2024

marcindsobczak Apr 4, 2024

marcindsobczak commented Apr 4, 2024

marcindsobczak commented Apr 4, 2024

pxyxyrus left a comment •

edited

marcindsobczak Apr 12, 2024

marcindsobczak Apr 12, 2024

marcindsobczak Apr 12, 2024

marcindsobczak Apr 23, 2024

marcindsobczak Apr 12, 2024

marcindsobczak Apr 12, 2024

marcindsobczak Apr 12, 2024

marcindsobczak Apr 12, 2024

marcindsobczak left a comment

marcindsobczak Apr 23, 2024

LukaszRozmej Apr 23, 2024

pxyxyrus Apr 25, 2024

marcindsobczak Apr 23, 2024

pxyxyrus Apr 25, 2024

marcindsobczak Apr 23, 2024

Fix/6790 return accepted for rejected tx added to broadcast #6889

Fix/6790 return accepted for rejected tx added to broadcast #6889

Conversation

pxyxyrus commented Apr 2, 2024

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaszRozmej Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcindsobczak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcindsobczak commented Apr 4, 2024

marcindsobczak commented Apr 4, 2024

pxyxyrus left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcindsobczak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaszRozmej Apr 2, 2024 •

edited

pxyxyrus left a comment •

edited