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.

Sign up for GitHub

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

Jump to bottom

Signature cache #1876

Merged
merged 18 commits into from
Dec 20, 2022
Merged

Signature cache #1876

merged 18 commits into from
Dec 20, 2022

Conversation

nagarev
Copy link
Contributor

@nagarev nagarev commented Sep 9, 2022 •
edited
Loading

Description

Extending Signature Cache support

Motivation and Context

#1850

How Has This Been Tested?

Unit Tests
QA

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

fed:signature-cache

Vovchyk
Vovchyk requested changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@Vovchyk Vovchyk left a comment

Choose a reason for hiding this comment

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

good start 👍🏻 . I guess we could make use the cache in other places too, what do you think?

@nagarev
Copy link
Contributor Author

nagarev commented Sep 13, 2022

Yes! there's a couple of places where we should include it too!

@nagarev nagarev requested review from Vovchyk and a team and removed request for a team September 15, 2022 14:41
Vovchyk
Vovchyk requested changes Sep 21, 2022
View reviewed changes
rskj-core/src/main/java/co/rsk/mine/MinerUtils.java Outdated
private static final Logger logger = LoggerFactory.getLogger("minerserver");

private final SignatureCache signatureCache;
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm.. this is a utility class, so ideally without any state, and all its methods should should be static

rskj-core/src/main/java/co/rsk/mine/MinerUtils.java
@@ -167,7 +172,7 @@ public List<org.ethereum.core.Transaction> getAllTransactions(TransactionPool tr

List<Transaction> txs = transactionPool.getPendingTransactions();

return PendingState.sortByPriceTakingIntoAccountSenderAndNonce(txs);
return PendingState.sortByPriceTakingIntoAccountSenderAndNonce(txs, signatureCache);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this method be static?

rskj-core/src/main/java/co/rsk/mine/MinerUtils.java
@@ -177,7 +182,7 @@ public List<org.ethereum.core.Transaction> filterTransactions(List<Transaction>
Keccak256 hash = tx.getHash();
Coin txValue = tx.getValue();
BigInteger txNonce = new BigInteger(1, tx.getNonce());
RskAddress txSender = tx.getSender();
RskAddress txSender = tx.getSender(signatureCache);
Copy link
Contributor

Choose a reason for hiding this comment

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

and this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update this as requested. 💪🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

rskj-core/src/main/java/co/rsk/net/handler/TxPendingValidator.java Outdated
BigInteger blockGasLimit = BigIntegers.fromUnsignedByteArray(executionBlock.getGasLimit());
Coin minimumGasPrice = executionBlock.getMinimumGasPrice();
long bestBlockNumber = executionBlock.getNumber();
long basicTxCost = tx.transactionCost(constants, activationConfig.forBlock(bestBlockNumber));

if (state == null && basicTxCost != 0) {
logger.trace("[tx={}, sender={}] account doesn't exist", tx.getHash(), tx.getSender());
logger.trace("[tx={}, sender={}] account doesn't exist", tx.getHash(), sender);
Copy link
Contributor

Choose a reason for hiding this comment

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

just an idea.. what about providing signature cache in the ctor and using it here? it's not that obvious that new param - sender - has to be sender of the tx being provided..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds good! if it's clearer let's do it! I thought that maybe it was too much to include this in the ctor just for a log, but you're right, it will look better. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@nagarev nagarev marked this pull request as ready for review October 4, 2022 03:32
@nagarev nagarev requested a review from a team as a code owner October 4, 2022 03:32
@nagarev nagarev requested a review from Vovchyk October 4, 2022 03:33
@nagarev
Copy link
Contributor Author

nagarev commented Oct 4, 2022

pipeline:run

1 similar comment
@nagarev
Copy link
Contributor Author

nagarev commented Oct 4, 2022

pipeline:run

Vovchyk
Vovchyk requested changes Oct 4, 2022
View reviewed changes
rskj-core/src/main/java/co/rsk/net/handler/TxPendingValidator.java Outdated

if (state == null && basicTxCost != 0) {
logger.trace("[tx={}, sender={}] account doesn't exist", tx.getHash(), tx.getSender());
logger.trace("[tx={}, sender={}] account doesn't exist", tx.getHash(), tx.getSender(signatureCache));
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could check logger.isTraceEnabled() before logging? cause tx.getSender(signatureCache) is heavy one if not cached

rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java Outdated
@@ -2666,10 +2670,10 @@ public BigInteger registerFlyoverBtcTransaction(
return BigInteger.valueOf(FlyoverTxResponseCodes.UNPROCESSABLE_TX_NOT_CONTRACT_ERROR.value());
}

if (!rskTx.getSender().equals(lbcAddress)) {
if (!rskTx.getSender(signatureCache).equals(lbcAddress)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we extract rskTx.getSender(signatureCache) into a local variable and reuse below?

rskj-core/src/main/java/co/rsk/RskContext.java Outdated
@@ -401,7 +401,7 @@ public TxQuotaChecker getTxQuotaChecker() {
checkIfNotClosed();

if (this.txQuotaChecker == null) {
this.txQuotaChecker = new TxQuotaChecker(System::currentTimeMillis);
this.txQuotaChecker = new TxQuotaChecker(System::currentTimeMillis, getBlockTxSignatureCache());
Copy link
Contributor

Choose a reason for hiding this comment

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

seems this checker is being used by tx pool.. shouldn't it be getReceivedTxSignatureCache()?

@nagarev nagarev requested a review from Vovchyk October 7, 2022 01:36
Vovchyk
Vovchyk previously approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@Vovchyk Vovchyk left a comment

Choose a reason for hiding this comment

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

lgtm. This may also need a counterpart pr for powpeg

@nagarev
Copy link
Contributor Author

nagarev commented Oct 7, 2022

pipeline:run

8 similar comments
@nagarev
Copy link
Contributor Author

nagarev commented Oct 7, 2022

pipeline:run

@Vovchyk
Copy link
Contributor

Vovchyk commented Oct 11, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Oct 11, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Oct 11, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Oct 11, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Oct 11, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Oct 11, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Oct 11, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Nov 24, 2022

pipeline:run

3 similar comments
@nagarev
Copy link
Contributor Author

nagarev commented Nov 24, 2022

pipeline:run

@nagarev
Copy link
Contributor Author

nagarev commented Nov 25, 2022

pipeline:run

@Vovchyk
Copy link
Contributor

Vovchyk commented Nov 25, 2022

pipeline:run

@sonarcloud
Copy link

sonarcloud bot commented Dec 16, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

94.0% 94.0% Coverage
0.0% 0.0% Duplication

@nagarev
Copy link
Contributor Author

nagarev commented Dec 19, 2022

pipeline:run

@Vovchyk Vovchyk merged commit bdb7a37 into master Dec 20, 2022
@Vovchyk Vovchyk deleted the signature-cache branch December 20, 2022 13:31
@aeidelman aeidelman added this to the Hop 4.3.0 milestone Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vovchyk Vovchyk Vovchyk approved these changes

@SergioDemianLerner SergioDemianLerner Awaiting requested review from SergioDemianLerner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Hop 4.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@nagarev @Vovchyk @aeidelman