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

Last nonce in pool for sender endpoint #4232

Merged
merged 4 commits into from
Jun 28, 2022

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented Jun 27, 2022

Changes

  • added new endpoint /pool/by-sender/last-nonce/:sender for the last nonce in tx pool for sender
  • small refactor on recently added logic on apiTransactionProcessor.go in order to reduce duplicate code

In order to test the changes, the following steps are needed:

  1. start a local testnet with this branch
  2. clone elrond-txgen-go and update the following:
    • in cmd/txgen/config/config.toml, leave Scenarios = ["badNonce"], update ProxyServerURL with the port that your local proxy uses, optional update MintingValue to a smaller one
    • copy from the newly generated node/config/walletKey.pem(local testnet) one of the private keys and paste it into cmd/txgen/config
  3. start txgen ./txgen --new-account --num-accounts 10
  4. run ./scripts/badNonce.sh
  5. make a Get request to the node: http://127.0.0.1:node-port/transaction/pool to get tx pool
  6. copy one hash and get the tx details via http://127.0.0.1:node-port/transaction/your-hash
  7. copy the tx sender and get last nonce from this new endpoint: http://127.0.0.1:node-port/transaction/pool/by-sender/last-nonce/your-sender

@sstanculeanu sstanculeanu self-assigned this Jun 27, 2022
@sstanculeanu sstanculeanu marked this pull request as ready for review June 27, 2022 14:40
@sstanculeanu sstanculeanu changed the title New endpoint for latest nonce in pool for sender New endpoint for last nonce in pool for sender Jun 27, 2022
@sstanculeanu sstanculeanu changed the title New endpoint for last nonce in pool for sender Last nonce in pool for sender endpoint Jun 27, 2022
@bogdan-rosianu bogdan-rosianu self-requested a review June 27, 2022 14:48
@ssd04 ssd04 self-requested a review June 27, 2022 14:53
senderShard := atp.shardCoordinator.ComputeId(senderAddr)
lastNonce, found := atp.fetchLastNonceForSender(string(senderAddr), senderShard)
if !found {
return 0, ErrCannotRetrieveNonce
Copy link
Contributor

Choose a reason for hiding this comment

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

if the sender doesn't have any tx in pool, is it an error? I don't think so. maybe treat this case and return "no transaction in pool for sender".. "cannot retrieve nonce" is not explanatory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed

return 0, ok
}

return lastTx.Tx.GetNonce(), ok
Copy link
Contributor

Choose a reason for hiding this comment

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

are the transactions in order by nonce? if no, this code doesn't cover all the situations.
If I send txs with nonces 1, 2, 3, 4, 7, 6, 5 in this order. will you code return 7 or 5 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the structure that holds the txs by sender keeps them sorted by nonce.. this unittest should cover exactly this situation, where lastNonce is added before nonce 1

@codecov-commenter
Copy link

Codecov Report

Merging #4232 (05f649a) into feat/new-tx-api (2e550f0) will increase coverage by 0.00%.
The diff coverage is 81.39%.

❗ Current head 05f649a differs from pull request most recent head 36311f8. Consider uploading reports for the commit 36311f8 to get more accurate results

@@               Coverage Diff                @@
##           feat/new-tx-api    #4232   +/-   ##
================================================
  Coverage            75.87%   75.87%           
================================================
  Files                  645      645           
  Lines                85294    85366   +72     
================================================
+ Hits                 64713    64768   +55     
- Misses               15785    15796   +11     
- Partials              4796     4802    +6     
Impacted Files Coverage Δ
storage/txcache/disabledCache.go 78.26% <0.00%> (-3.56%) ⬇️
facade/initial/initialNodeFacade.go 80.95% <66.66%> (+0.30%) ⬆️
...external/transactionAPI/apiTransactionProcessor.go 85.48% <73.17%> (-1.79%) ⬇️
storage/txcache/txCache.go 94.15% <77.77%> (-1.02%) ⬇️
api/groups/transactionGroup.go 78.95% <100.00%> (+1.20%) ⬆️
facade/nodeFacade.go 79.12% <100.00%> (+0.15%) ⬆️
node/external/nodeApiResolver.go 62.13% <100.00%> (+0.74%) ⬆️
storage/txcache/crossTxCache.go 56.89% <100.00%> (+1.53%) ⬆️
...t/processor/peerAuthenticationRequestsProcessor.go 94.40% <0.00%> (-2.40%) ⬇️
process/block/baseProcess.go 73.06% <0.00%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e550f0...36311f8. Read the comment docs.

ssd04
ssd04 previously approved these changes Jun 28, 2022
@sstanculeanu sstanculeanu merged commit 4cf8fa1 into feat/new-tx-api Jun 28, 2022
@sstanculeanu sstanculeanu deleted the last_nonce_for_sender branch June 28, 2022 08:17
@sstanculeanu sstanculeanu added the ignore-for-release-notes Do not include item in release notes label Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release-notes Do not include item in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants