Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

pxyxyrus
Copy link
Contributor

@pxyxyrus pxyxyrus commented Apr 2, 2024

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.

{
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happened to _broadcaster.Broadcast call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@LukaszRozmej LukaszRozmej Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@marcindsobczak marcindsobczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 453 to 461
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

[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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1619 to 1622
_txPool.SubmitTx(tx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.Accepted);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same story as in test Should_not_replace_better_txs_by_worse_ones()

Comment on lines 1594 to 1630
[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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

@pxyxyrus pxyxyrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Comment on lines 116 to 128
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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
  2. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry I reviewed on wrong commit

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth to consider before merging

Comment on lines 112 to 125
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 116 to 118
if (tx is not null
&& _persistentTxs.TryInsert(tx.Hash, tx.SupportsBlobs ? new LightTransaction(tx) : tx)
&& (tx.MaxFeePerGas >= _baseFeeThreshold || tx.IsFree()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@marcindsobczak marcindsobczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[TestCase]
public void Should_only_add_legacy_and_1559_local_transactions_to_local_pool_when_underpaid()
[TestCase(TxType.Legacy, true)]
[TestCase(TxType.EIP1559, true)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth to consider before merging

@LukaszRozmej LukaszRozmej merged commit 6a55a67 into NethermindEth:master May 13, 2024
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants