Skip to content

Commit

Permalink
Layered txpool: fix for unsent drop notifications on remove (#7538)
Browse files Browse the repository at this point in the history
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
  • Loading branch information
fab-10 authored Sep 5, 2024
1 parent edd3c4f commit dc6324c
Show file tree
Hide file tree
Showing 17 changed files with 682 additions and 308 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

### Bug fixes
- Layered txpool: do not send notifications when moving tx between layers [#7539](https://github.com/hyperledger/besu/pull/7539)
- Layered txpool: fix for unsent drop notifications on remove [#7538](https://github.com/hyperledger/besu/pull/7538)

## 24.9.0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package org.hyperledger.besu.ethereum.eth.transactions;

import org.hyperledger.besu.datatypes.TransactionType;
import org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.AddReason;
import org.hyperledger.besu.ethereum.eth.transactions.layered.AddReason;
import org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason;
import org.hyperledger.besu.metrics.BesuMetricCategory;
import org.hyperledger.besu.metrics.ReplaceableDoubleSupplier;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,17 @@
*/
package org.hyperledger.besu.ethereum.eth.transactions.layered;

import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.AddReason.MOVE;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.EVICTED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.FOLLOW_INVALIDATED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.AddReason.MOVE;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.LayerMoveReason.EVICTED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.LayerMoveReason.FOLLOW_INVALIDATED;

import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.ethereum.eth.manager.EthScheduler;
import org.hyperledger.besu.ethereum.eth.transactions.BlobCache;
import org.hyperledger.besu.ethereum.eth.transactions.PendingTransaction;
import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolMetrics;
import org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason;

import java.util.Map;
import java.util.NavigableMap;
Expand All @@ -45,7 +46,7 @@ public AbstractSequentialTransactionsLayer(
}

@Override
public void remove(final PendingTransaction invalidatedTx, final RemovalReason reason) {
public void remove(final PendingTransaction invalidatedTx, final PoolRemovalReason reason) {
nextLayer.remove(invalidatedTx, reason);

final var senderTxs = txsBySender.get(invalidatedTx.getSender());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.ALREADY_KNOWN;
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.REJECTED_UNDERPRICED_REPLACEMENT;
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.TRY_NEXT_LAYER;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.AddReason.MOVE;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.CONFIRMED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.CROSS_LAYER_REPLACED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.EVICTED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.REPLACED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.AddReason.MOVE;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.LayerMoveReason.EVICTED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason.CONFIRMED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason.CROSS_LAYER_REPLACED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason.REPLACED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.RemovedFrom.POOL;

import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.Hash;
Expand Down Expand Up @@ -292,7 +293,8 @@ public PendingTransaction promoteFor(

if (remainingPromotionsPerType[txType.ordinal()] > 0) {
senderTxs.pollFirstEntry();
processRemove(senderTxs, candidateTx.getTransaction(), RemovalReason.PROMOTED);
processRemove(
senderTxs, candidateTx.getTransaction(), RemovalReason.LayerMoveReason.PROMOTED);
metrics.incrementRemoved(candidateTx, "promoted", name());

if (senderTxs.isEmpty()) {
Expand Down Expand Up @@ -419,6 +421,9 @@ protected PendingTransaction processRemove(
decreaseCounters(removedTx);
metrics.incrementRemoved(removedTx, removalReason.label(), name());
internalRemove(senderTxs, removedTx, removalReason);
if (removalReason.removedFrom().equals(POOL)) {
notifyTransactionDropped(removedTx);
}
}
return removedTx;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.ethereum.eth.transactions.layered;

import java.util.Locale;

/** Describe why we are trying to add a tx to a layer. */
public enum AddReason {
/** When adding a tx, that is not present in the pool. */
NEW(true, true),
/** When adding a tx as result of an internal move between layers. */
MOVE(false, false),
/** When adding a tx as result of a promotion from a lower layer. */
PROMOTED(false, false);

private final boolean sendNotification;
private final boolean makeCopy;
private final String label;

AddReason(final boolean sendNotification, final boolean makeCopy) {
this.sendNotification = sendNotification;
this.makeCopy = makeCopy;
this.label = name().toLowerCase(Locale.ROOT);
}

/**
* Should we send add notification for this reason?
*
* @return true if notification should be sent
*/
public boolean sendNotification() {
return sendNotification;
}

/**
* Should the layer make a copy of the pending tx before adding it, to avoid keeping reference to
* potentially large underlying byte buffers?
*
* @return true is a copy is necessary
*/
public boolean makeCopy() {
return makeCopy;
}

/**
* Return a label that identify this reason to be used in the metric system.
*
* @return a label
*/
public String label() {
return label;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
*/
package org.hyperledger.besu.ethereum.eth.transactions.layered;

import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.AddReason.MOVE;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.BELOW_BASE_FEE;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.AddReason.MOVE;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.LayerMoveReason.DEMOTED;

import org.hyperledger.besu.datatypes.Wei;
import org.hyperledger.besu.ethereum.core.BlockHeader;
Expand Down Expand Up @@ -105,7 +105,7 @@ protected void internalBlockAdded(final BlockHeader blockHeader, final FeeMarket
while (itTxsBySender.hasNext()) {
final var senderTxs = itTxsBySender.next().getValue();

Optional<Long> maybeFirstUnderpricedNonce = Optional.empty();
Optional<Long> maybeFirstDemotedNonce = Optional.empty();

for (final var e : senderTxs.entrySet()) {
final PendingTransaction tx = e.getValue();
Expand All @@ -115,25 +115,27 @@ protected void internalBlockAdded(final BlockHeader blockHeader, final FeeMarket
} else {
// otherwise sender txs starting from this nonce need to be demoted to next layer,
// and we can go to next sender
maybeFirstUnderpricedNonce = Optional.of(e.getKey());
maybeFirstDemotedNonce = Optional.of(e.getKey());
break;
}
}

maybeFirstUnderpricedNonce.ifPresent(
maybeFirstDemotedNonce.ifPresent(
nonce -> {
// demote all txs after the first underpriced to the next layer, because none of them is
// demote all txs after the first demoted to the next layer, because none of them is
// executable now, and we can avoid sorting them until they are candidate for execution
// again
final var demoteTxs = senderTxs.tailMap(nonce, true);
while (!demoteTxs.isEmpty()) {
final PendingTransaction demoteTx = demoteTxs.pollLastEntry().getValue();
LOG.atTrace()
.setMessage("Demoting tx {} with max gas price below next block base fee {}")
.setMessage(
"Demoting tx {} since it does not respect anymore the requisites to stay in this layer."
+ " Next block base fee {}")
.addArgument(demoteTx::toTraceLog)
.addArgument(newNextBlockBaseFee::toHumanReadableString)
.log();
processEvict(senderTxs, demoteTx, BELOW_BASE_FEE);
processEvict(senderTxs, demoteTx, DEMOTED);
addToNextLayer(senderTxs, demoteTx, 0, MOVE);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*/
package org.hyperledger.besu.ethereum.eth.transactions.layered;

import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.DROPPED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason.DROPPED;

import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.Hash;
Expand All @@ -25,6 +25,7 @@
import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactionDroppedListener;
import org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult;
import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolMetrics;
import org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason;
import org.hyperledger.besu.ethereum.mainnet.feemarket.FeeMarket;
import org.hyperledger.besu.util.Subscribers;

Expand Down Expand Up @@ -84,7 +85,7 @@ public TransactionAddedResult add(
}

@Override
public void remove(final PendingTransaction pendingTransaction, final RemovalReason reason) {}
public void remove(final PendingTransaction pendingTransaction, final PoolRemovalReason reason) {}

@Override
public void penalize(final PendingTransaction penalizedTx) {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.ALREADY_KNOWN;
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.INTERNAL_ERROR;
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.AddReason.NEW;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.INVALIDATED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.RECONCILED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.AddReason.NEW;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason.INVALIDATED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.PoolRemovalReason.RECONCILED;

import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.Hash;
Expand Down Expand Up @@ -316,7 +316,6 @@ public synchronized List<Transaction> getPriorityTransactions() {

@Override
public void selectTransactions(final PendingTransactions.TransactionSelector selector) {
final List<PendingTransaction> invalidTransactions = new ArrayList<>();
final List<PendingTransaction> penalizedTransactions = new ArrayList<>();
final Set<Address> skipSenders = new HashSet<>();

Expand Down Expand Up @@ -347,7 +346,12 @@ public void selectTransactions(final PendingTransactions.TransactionSelector sel
.log();

if (selectionResult.discard()) {
invalidTransactions.add(candidatePendingTx);
ethScheduler.scheduleTxWorkerTask(
() -> {
synchronized (this) {
prioritizedTransactions.remove(candidatePendingTx, INVALIDATED);
}
});
logDiscardedTransaction(candidatePendingTx, selectionResult);
}

Expand Down Expand Up @@ -377,20 +381,13 @@ public void selectTransactions(final PendingTransactions.TransactionSelector sel
}

ethScheduler.scheduleTxWorkerTask(
() -> {
invalidTransactions.forEach(
invalidTx -> {
synchronized (this) {
prioritizedTransactions.remove(invalidTx, INVALIDATED);
}
});
penalizedTransactions.forEach(
penalizedTx -> {
synchronized (this) {
prioritizedTransactions.internalPenalize(penalizedTx);
}
});
});
() ->
penalizedTransactions.forEach(
penalizedTx -> {
synchronized (this) {
prioritizedTransactions.penalize(penalizedTx);
}
}));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*/
package org.hyperledger.besu.ethereum.eth.transactions.layered;

import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.PROMOTED;
import static org.hyperledger.besu.ethereum.eth.transactions.layered.RemovalReason.LayerMoveReason.PROMOTED;

import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.ethereum.core.BlockHeader;
Expand Down
Loading

0 comments on commit dc6324c

Please sign in to comment.