Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-2850] Create a transaction pool configuration object #1615

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import tech.pegasys.pantheon.controller.PantheonControllerBuilder;
import tech.pegasys.pantheon.ethereum.eth.EthereumWireProtocolConfiguration;
import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration;
import tech.pegasys.pantheon.ethereum.eth.transactions.PendingTransactions;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolConfiguration;
import tech.pegasys.pantheon.ethereum.graphql.GraphQLConfiguration;
import tech.pegasys.pantheon.ethereum.p2p.peers.EnodeURL;
import tech.pegasys.pantheon.ethereum.permissioning.PermissioningConfiguration;
Expand Down Expand Up @@ -114,8 +114,7 @@ public void startNode(final PantheonNode node) {
.privacyParameters(node.getPrivacyParameters())
.nodePrivateKeyFile(KeyPairUtil.getDefaultKeyFile(node.homeDirectory()))
.metricsSystem(noOpMetricsSystem)
.maxPendingTransactions(PendingTransactions.MAX_PENDING_TRANSACTIONS)
.pendingTransactionRetentionPeriod(PendingTransactions.DEFAULT_TX_RETENTION_HOURS)
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.rocksDbConfiguration(new RocksDbConfiguration.Builder().databaseDir(tempDir).build())
.ethereumWireProtocolConfiguration(EthereumWireProtocolConfiguration.defaultConfig())
.clock(Clock.systemUTC())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* Copyright 2019 ConsenSys AG.
*
* 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.
*/
package tech.pegasys.pantheon.ethereum.eth.transactions;

import java.util.Objects;

public class TransactionPoolConfiguration {

private final int txPoolMaxSize;
private final int pendingTxRetentionPeriod;
private final int txMessageKeepAliveSeconds;

public TransactionPoolConfiguration(
final int txPoolMaxSize,
final int pendingTxRetentionPeriod,
final int txMessageKeepAliveSeconds) {
this.txPoolMaxSize = txPoolMaxSize;
this.pendingTxRetentionPeriod = pendingTxRetentionPeriod;
this.txMessageKeepAliveSeconds = txMessageKeepAliveSeconds;
}

public int getTxPoolMaxSize() {
return txPoolMaxSize;
}

public int getPendingTxRetentionPeriod() {
return pendingTxRetentionPeriod;
}

public int getTxMessageKeepAliveSeconds() {
return txMessageKeepAliveSeconds;
}

@Override
public boolean equals(final Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
final TransactionPoolConfiguration that = (TransactionPoolConfiguration) o;
return txPoolMaxSize == that.txPoolMaxSize
&& Objects.equals(pendingTxRetentionPeriod, that.pendingTxRetentionPeriod)
&& Objects.equals(txMessageKeepAliveSeconds, that.txMessageKeepAliveSeconds);
}

@Override
public int hashCode() {
return Objects.hash(txPoolMaxSize, pendingTxRetentionPeriod, txMessageKeepAliveSeconds);
}

@Override
public String toString() {
return "TransactionPoolConfiguration{"
+ "txPoolMaxSize="
+ txPoolMaxSize
+ ", pendingTxRetentionPeriod="
+ pendingTxRetentionPeriod
+ ", txMessageKeepAliveSeconds="
+ txMessageKeepAliveSeconds
+ '}';
}

public static Builder builder() {
return new Builder();
}

public static class Builder {
private int txPoolMaxSize = PendingTransactions.MAX_PENDING_TRANSACTIONS;
private int pendingTxRetentionPeriod = PendingTransactions.DEFAULT_TX_RETENTION_HOURS;
private int txMessageKeepAliveSeconds = TransactionPool.DEFAULT_TX_MSG_KEEP_ALIVE;

public Builder txPoolMaxSize(final int txPoolMaxSize) {
this.txPoolMaxSize = txPoolMaxSize;
return this;
}

public Builder pendingTxRetentionPeriod(final int pendingTxRetentionPeriod) {
this.pendingTxRetentionPeriod = pendingTxRetentionPeriod;
return this;
}

public Builder txMessageKeepAliveSeconds(final int txMessageKeepAliveSeconds) {
this.txMessageKeepAliveSeconds = txMessageKeepAliveSeconds;
return this;
}

public TransactionPoolConfiguration build() {
return new TransactionPoolConfiguration(
txPoolMaxSize, pendingTxRetentionPeriod, txMessageKeepAliveSeconds);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,17 @@ public static TransactionPool createTransactionPool(
final ProtocolContext<?> protocolContext,
final EthContext ethContext,
final Clock clock,
final int maxPendingTransactions,
final MetricsSystem metricsSystem,
final SyncState syncState,
final int maxTransactionRetentionHours,
final Wei minTransactionGasPrice,
Copy link
Contributor

Choose a reason for hiding this comment

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

(Discussion) What about folding minTransactionGasPrice into TransactionPoolConfiguration? Or maybe the TransactionPool also gets access to MiningParams so that we don't end up duplicating data on 2 different config objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it makes sense.

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 mean the second approach where TransactionPool also gets access to MiningParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the PantheonControllerBuilder passes the minTransactionPrice to the transactionPool from the MiningParams

    final TransactionPool transactionPool =
        TransactionPoolFactory.createTransactionPool(
            protocolSchedule,
            protocolContext,
            ethProtocolManager.ethContext(),
            clock,
            metricsSystem,
            syncState,
            miningParameters.getMinTransactionGasPrice(),
            transactionPoolConfiguration)

SInce it uses only one parameter from the MiningParams i don't think we need to give it access to the whole object now. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♀ Just feels odd to have a config object and then have an additional dangling option. But there's some weirdness with all 3 options really (as is + my 2 alternative suggestions). I think I'm leaning towards passing in the MiningParams - it makes it more explicit that the transactionPool's behavior is modified by the mining configuration. But I'll leave it up to you.

final int txMessageKeepAliveSeconds) {
final TransactionPoolConfiguration transactionPoolConfiguration) {

final PendingTransactions pendingTransactions =
new PendingTransactions(
maxTransactionRetentionHours, maxPendingTransactions, clock, metricsSystem);
transactionPoolConfiguration.getPendingTxRetentionPeriod(),
transactionPoolConfiguration.getTxPoolMaxSize(),
clock,
metricsSystem);

final PeerTransactionTracker transactionTracker = new PeerTransactionTracker();
final TransactionsMessageSender transactionsMessageSender =
Expand Down Expand Up @@ -67,7 +68,7 @@ public static TransactionPool createTransactionPool(
PantheonMetricCategory.TRANSACTION_POOL,
"transactions_messages_skipped_total",
"Total number of transactions messages skipped by the processor.")),
txMessageKeepAliveSeconds);
transactionPoolConfiguration.getTxMessageKeepAliveSeconds());

ethContext.getEthMessages().subscribe(EthPV62.TRANSACTIONS, transactionsMessageHandler);
protocolContext.getBlockchain().observeBlockAdded(transactionPool);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@
import tech.pegasys.pantheon.ethereum.eth.messages.StatusMessage;
import tech.pegasys.pantheon.ethereum.eth.messages.TransactionsMessage;
import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
import tech.pegasys.pantheon.ethereum.eth.transactions.PendingTransactions;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPool;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolConfiguration;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolFactory;
import tech.pegasys.pantheon.ethereum.mainnet.MainnetProtocolSchedule;
import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule;
Expand Down Expand Up @@ -1079,12 +1078,10 @@ public void transactionMessagesGoToTheCorrectExecutor() {
protocolContext,
ethManager.ethContext(),
TestClock.fixed(),
PendingTransactions.MAX_PENDING_TRANSACTIONS,
metricsSystem,
mock(SyncState.class),
PendingTransactions.DEFAULT_TX_RETENTION_HOURS,
Wei.ZERO,
TransactionPool.DEFAULT_TX_MSG_KEEP_ALIVE);
TransactionPoolConfiguration.builder().build());

// Send just a transaction message.
final PeerConnection peer = setupPeer(ethManager, (cap, msg, connection) -> {});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,12 +148,10 @@ public TestNode(
protocolContext,
ethContext,
TestClock.fixed(),
PendingTransactions.MAX_PENDING_TRANSACTIONS,
metricsSystem,
syncState,
PendingTransactions.DEFAULT_TX_RETENTION_HOURS,
Wei.ZERO,
TransactionPool.DEFAULT_TX_MSG_KEEP_ALIVE);
TransactionPoolConfiguration.builder().build());

networkRunner.start();
selfPeer = DefaultPeer.fromEnodeURL(network.getLocalEnode().get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import tech.pegasys.pantheon.ethereum.eth.sync.TrailingPeerRequirements;
import tech.pegasys.pantheon.ethereum.eth.transactions.PendingTransactions;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPool;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolConfiguration;
import tech.pegasys.pantheon.ethereum.graphql.GraphQLConfiguration;
import tech.pegasys.pantheon.ethereum.jsonrpc.JsonRpcConfiguration;
import tech.pegasys.pantheon.ethereum.jsonrpc.RpcApi;
Expand Down Expand Up @@ -823,9 +824,12 @@ PantheonController<?> buildController() {
.dataDirectory(dataDir())
.miningParameters(
new MiningParameters(coinbase, minTransactionGasPrice, extraData, isMiningEnabled))
.maxPendingTransactions(txPoolMaxSize)
.pendingTransactionRetentionPeriod(pendingTxRetentionPeriod)
.txMessageKeepAliveSeconds(txMessageKeepAliveSeconds)
.transactionPoolConfiguration(
TransactionPoolConfiguration.builder()
.txPoolMaxSize(txPoolMaxSize)
.pendingTxRetentionPeriod(pendingTxRetentionPeriod)
.txMessageKeepAliveSeconds(txMessageKeepAliveSeconds)
.build())
.nodePrivateKeyFile(nodePrivateKeyFile())
.metricsSystem(metricsSystem.get())
.privacyParameters(privacyParameters())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration;
import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPool;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolConfiguration;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolFactory;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.methods.JsonRpcMethodFactory;
import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule;
Expand Down Expand Up @@ -66,15 +67,13 @@ public abstract class PantheonControllerBuilder<C> {
protected SynchronizerConfiguration syncConfig;
protected EthProtocolManager ethProtocolManager;
protected EthereumWireProtocolConfiguration ethereumWireProtocolConfiguration;
protected TransactionPoolConfiguration transactionPoolConfiguration;
protected Integer networkId;
protected MiningParameters miningParameters;
protected MetricsSystem metricsSystem;
protected PrivacyParameters privacyParameters;
protected Path dataDirectory;
protected Clock clock;
protected Integer maxPendingTransactions;
protected Integer pendingTransactionRetentionPeriod;
protected Integer txMessageKeepAliveSeconds = TransactionPool.DEFAULT_TX_MSG_KEEP_ALIVE;
protected KeyPair nodeKeys;
private StorageProvider storageProvider;
private final List<Runnable> shutdownActions = new ArrayList<>();
Expand Down Expand Up @@ -149,20 +148,9 @@ public PantheonControllerBuilder<C> clock(final Clock clock) {
return this;
}

public PantheonControllerBuilder<C> maxPendingTransactions(final int maxPendingTransactions) {
this.maxPendingTransactions = maxPendingTransactions;
return this;
}

public PantheonControllerBuilder<C> pendingTransactionRetentionPeriod(
final int pendingTransactionRetentionPeriod) {
this.pendingTransactionRetentionPeriod = pendingTransactionRetentionPeriod;
return this;
}

public PantheonControllerBuilder<C> txMessageKeepAliveSeconds(
final int txMessageKeepAliveSeconds) {
this.txMessageKeepAliveSeconds = txMessageKeepAliveSeconds;
public PantheonControllerBuilder<C> transactionPoolConfiguration(
final TransactionPoolConfiguration transactionPoolConfiguration) {
this.transactionPoolConfiguration = transactionPoolConfiguration;
return this;
}

Expand All @@ -176,7 +164,7 @@ public PantheonController<C> build() throws IOException {
checkNotNull(privacyParameters, "Missing privacy parameters");
checkNotNull(dataDirectory, "Missing data directory"); // Why do we need this?
checkNotNull(clock, "Mising clock");
checkNotNull(maxPendingTransactions, "Missing max pending transactions");
checkNotNull(transactionPoolConfiguration, "Missing transaction pool configuration");
checkNotNull(nodeKeys, "Missing node keys");
checkArgument(
storageProvider != null || rocksDbConfiguration != null,
Expand Down Expand Up @@ -238,12 +226,10 @@ public PantheonController<C> build() throws IOException {
protocolContext,
ethProtocolManager.ethContext(),
clock,
maxPendingTransactions,
metricsSystem,
syncState,
pendingTransactionRetentionPeriod,
miningParameters.getMinTransactionGasPrice(),
txMessageKeepAliveSeconds);
transactionPoolConfiguration);

final MiningCoordinator miningCoordinator =
createMiningCoordinator(
Expand Down
5 changes: 2 additions & 3 deletions pantheon/src/test/java/tech/pegasys/pantheon/PrivacyTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import tech.pegasys.pantheon.ethereum.core.PrivacyParameters;
import tech.pegasys.pantheon.ethereum.eth.EthereumWireProtocolConfiguration;
import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration;
import tech.pegasys.pantheon.ethereum.eth.transactions.PendingTransactions;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolConfiguration;
import tech.pegasys.pantheon.ethereum.mainnet.PrecompiledContract;
import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
import tech.pegasys.pantheon.testutil.TestClock;
Expand Down Expand Up @@ -63,8 +63,7 @@ public void privacyPrecompiled() throws IOException {
.dataDirectory(dataDir)
.clock(TestClock.fixed())
.privacyParameters(privacyParameters)
.maxPendingTransactions(PendingTransactions.MAX_PENDING_TRANSACTIONS)
.pendingTransactionRetentionPeriod(PendingTransactions.DEFAULT_TX_RETENTION_HOURS)
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.build();

final Address privacyContractAddress = Address.privacyPrecompiled(ADDRESS);
Expand Down
11 changes: 4 additions & 7 deletions pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import tech.pegasys.pantheon.ethereum.eth.EthereumWireProtocolConfiguration;
import tech.pegasys.pantheon.ethereum.eth.sync.SyncMode;
import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration;
import tech.pegasys.pantheon.ethereum.eth.transactions.PendingTransactions;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolConfiguration;
import tech.pegasys.pantheon.ethereum.graphql.GraphQLConfiguration;
import tech.pegasys.pantheon.ethereum.jsonrpc.JsonRpcConfiguration;
import tech.pegasys.pantheon.ethereum.jsonrpc.websocket.WebSocketConfiguration;
Expand Down Expand Up @@ -132,9 +132,8 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
.metricsSystem(noOpMetricsSystem)
.privacyParameters(PrivacyParameters.DEFAULT)
.clock(TestClock.fixed())
.maxPendingTransactions(PendingTransactions.MAX_PENDING_TRANSACTIONS)
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.storageProvider(createKeyValueStorageProvider(dbAhead))
.pendingTransactionRetentionPeriod(PendingTransactions.DEFAULT_TX_RETENTION_HOURS)
.build()) {
setupState(blockCount, controller.getProtocolSchedule(), controller.getProtocolContext());
}
Expand All @@ -152,9 +151,8 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
.metricsSystem(noOpMetricsSystem)
.privacyParameters(PrivacyParameters.DEFAULT)
.clock(TestClock.fixed())
.maxPendingTransactions(PendingTransactions.MAX_PENDING_TRANSACTIONS)
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.storageProvider(createKeyValueStorageProvider(dbAhead))
.pendingTransactionRetentionPeriod(PendingTransactions.DEFAULT_TX_RETENTION_HOURS)
.build();
final String listenHost = InetAddress.getLoopbackAddress().getHostAddress();
final JsonRpcConfiguration aheadJsonRpcConfiguration = jsonRpcConfiguration();
Expand Down Expand Up @@ -212,8 +210,7 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
.metricsSystem(noOpMetricsSystem)
.privacyParameters(PrivacyParameters.DEFAULT)
.clock(TestClock.fixed())
.maxPendingTransactions(PendingTransactions.MAX_PENDING_TRANSACTIONS)
.pendingTransactionRetentionPeriod(PendingTransactions.DEFAULT_TX_RETENTION_HOURS)
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.build();
final EnodeURL enode = runnerAhead.getLocalEnode().get();
final EthNetworkConfig behindEthNetworkConfiguration =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import tech.pegasys.pantheon.ethereum.eth.manager.EthProtocolManager;
import tech.pegasys.pantheon.ethereum.eth.sync.BlockBroadcaster;
import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration;
import tech.pegasys.pantheon.ethereum.eth.transactions.TransactionPoolConfiguration;
import tech.pegasys.pantheon.ethereum.graphql.GraphQLConfiguration;
import tech.pegasys.pantheon.ethereum.jsonrpc.JsonRpcConfiguration;
import tech.pegasys.pantheon.ethereum.jsonrpc.websocket.WebSocketConfiguration;
Expand Down Expand Up @@ -111,6 +112,7 @@ public abstract class CommandTestAbstract {
@Captor ArgumentCaptor<WebSocketConfiguration> wsRpcConfigArgumentCaptor;
@Captor ArgumentCaptor<MetricsConfiguration> metricsConfigArgumentCaptor;
@Captor ArgumentCaptor<PermissioningConfiguration> permissioningConfigurationArgumentCaptor;
@Captor ArgumentCaptor<TransactionPoolConfiguration> transactionPoolConfigurationArgumentCaptor;

@Rule public final TemporaryFolder temp = new TemporaryFolder();

Expand All @@ -131,10 +133,8 @@ public void initMocks() throws Exception {
when(mockControllerBuilder.rocksDbConfiguration(any())).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.dataDirectory(any())).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.miningParameters(any())).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.maxPendingTransactions(anyInt())).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.pendingTransactionRetentionPeriod(anyInt()))
.thenReturn(mockControllerBuilder);
when(mockControllerBuilder.txMessageKeepAliveSeconds(anyInt()))
when(mockControllerBuilder.transactionPoolConfiguration(
any(TransactionPoolConfiguration.class)))
.thenReturn(mockControllerBuilder);
when(mockControllerBuilder.nodePrivateKeyFile(any())).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.metricsSystem(any())).thenReturn(mockControllerBuilder);
Expand Down
Loading