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

Changetxpool_beusPendingTransactions:numResults from a required parameter to an optional parameter #6708

Merged
merged 16 commits into from
Mar 14, 2024

Conversation

MASDXI
Copy link
Contributor

@MASDXI MASDXI commented Mar 11, 2024

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Most advanced CI tests are deferred until PR approval, but you could:

  • locally run all unit tests via: ./gradlew build
  • locally run all acceptance tests via: ./gradlew acceptanceTest
  • locally run all integration tests via: ./gradlew integrationTest
  • locally run all reference tests via: ./gradlew ethereum:referenceTests:referenceTests

PR description

Enhancement TX_POOL API txpool_besuPendingTransactions by changing 'numResults' from a required parameter to an optional parameter. if the parameter limit does not exist will use pending transactions size as a limit.

Fixed Issue(s)

fixes #6707

@MASDXI MASDXI changed the title Change 'limit' from a required parameter to an optional parameter Changetxpool_beusPendingTransactions: limit from a required parameter to an optional parameter Mar 11, 2024
@MASDXI MASDXI changed the title Changetxpool_beusPendingTransactions: limit from a required parameter to an optional parameter Changetxpool_beusPendingTransactions:limit from a required parameter to an optional parameter Mar 11, 2024
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM - @fab-10 any comments?

MASDXI and others added 11 commits March 13, 2024 11:43
…yperledger#6702)

* relax JsonCallParameter constructor to allow for both input and data being set if equal

Signed-off-by: Friedemann Fürst <[email protected]>

* fix: format

Signed-off-by: Friedemann Fürst <[email protected]>

* add changelog entry

Signed-off-by: Friedemann Fürst <[email protected]>

---------

Signed-off-by: Friedemann Fürst <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: MASDXI <[email protected]>
* added more spec tests

Signed-off-by: Sally MacFarlane <[email protected]>

* fixed typo

Signed-off-by: Sally MacFarlane <[email protected]>

* fixed error message

Signed-off-by: Sally MacFarlane <[email protected]>

---------

Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: MASDXI <[email protected]>
…e it (hyperledger#6675)

* Don't start a BFT mining coordinator when it is created, just enable it

Signed-off-by: Matthew Whitehead <[email protected]>

* Update change log

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: MASDXI <[email protected]>
* make artifacts more snapshot friendly
* break out new workflows for snapshots, and a develop releease
* removes checking for approval, runs on pr update
* adds concurrency so updated refs cancel prior runs if still running
* explicitly disable caching on gradle setup tasks
---------

Signed-off-by: Justin Florentine <[email protected]>
Signed-off-by: MASDXI <[email protected]>
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

The change makes sense, a couple of suggestions and please add unit tests to validate the change.

Note that since the txpool could contain many MB of data, and if requesting all txs, it is still possible that are limits elsewhere (HTTP, proxies, etc...) that could prevent to get the full response

@MASDXI MASDXI changed the title Changetxpool_beusPendingTransactions:limit from a required parameter to an optional parameter Changetxpool_beusPendingTransactions:numResults from a required parameter to an optional parameter Mar 14, 2024
MASDXI and others added 3 commits March 14, 2024 09:50
reuse 'pendingTransactions' to avoid fetching txs in pool twice

Signed-off-by: MASDXI <[email protected]>
Signed-off-by: MASDXI <[email protected]>
@fab-10 fab-10 enabled auto-merge (squash) March 14, 2024 09:14
@fab-10 fab-10 merged commit 9bf5427 into hyperledger:main Mar 14, 2024
42 checks passed
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
…arameter to an optional parameter (hyperledger#6708)

Signed-off-by: MASDXI <[email protected]>
Signed-off-by: amsmota <[email protected]>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
…arameter to an optional parameter (hyperledger#6708)

Signed-off-by: MASDXI <[email protected]>
Signed-off-by: amsmota <[email protected]>
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
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.

Change numResult parameter to optional in txpool_besuPendingTransactions
8 participants