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

Remove getbyblocknumber from flexibleprivacyprecompiledcontract #5221

Conversation

gfukushima
Copy link
Contributor

@gfukushima gfukushima commented Mar 15, 2023

PR description

This PR aims to remove the usage of getByBlockNumber from the FlexiblePrivacyPrecompiledContract. As far as analysis shows the only requirement is to get a protocolSpec (it doesn't matter which one) currently it always fetches the protocolSpec for block number 1 which can vary according to the network config. We're changing that get the protocolSpec for block 0 which is the only block we always have the header.

AT nonmainnet on circle CI
Pipeline - 21048

Fixed Issue(s)

Fixes #5160

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Acceptance Tests (Non Mainnet)

  • I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.

Changelog

@gfukushima gfukushima force-pushed the remove-getbyblocknumber-from-FlexiblePrivacyPrecompiledContract branch from 5b4bc58 to c9719d1 Compare March 16, 2023 04:42
@gfukushima gfukushima marked this pull request as ready for review March 16, 2023 04:43
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.

looks ok to me. block 0 vs block 1 isn't going to make any difference I don't think

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@@ -1344,17 +1347,19 @@ private void createLogsSubscriptionService(

private void createPrivateTransactionObserver(
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know how feasible this is, but adding a test for this would be good. But it might already be covered by a test at another level.

@gfukushima gfukushima added this pull request to the merge queue Mar 16, 2023
Merged via the queue into hyperledger:main with commit b46d903 Mar 16, 2023
@gfukushima gfukushima deleted the remove-getbyblocknumber-from-FlexiblePrivacyPrecompiledContract branch March 16, 2023 07:41
@gfukushima gfukushima added the TeamGroot GH issues worked on by Groot Team label Mar 24, 2023
@gfukushima gfukushima self-assigned this Mar 24, 2023
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
…rledger#5221)

<!-- Thanks for sending a pull request! Please check out our
contribution guidelines: -->
<!-- https://github.com/hyperledger/besu/blob/main/CONTRIBUTING.md -->

## PR description
This PR aims to remove the usage of getByBlockNumber from the
FlexiblePrivacyPrecompiledContract. As far as analysis shows the only
requirement is to get a protocolSpec (it doesn't matter which one)
currently it always fetches the protocolSpec for block number 1 which
can vary according to the network config. We're changing that get the
protocolSpec for block 0 which is the only block we always have the
header.

AT nonmainnet on circle CI
Pipeline - 21048

## Fixed Issue(s)
<!-- Please link to fixed issue(s) here using format: fixes #<issue
number> -->
<!-- Example: "fixes #2" -->
Fixes hyperledger#5160 

## Documentation

- [x] I thought about documentation and added the `doc-change-required`
label to this PR if
[updates are
required](https://wiki.hyperledger.org/display/BESU/Documentation).

## Acceptance Tests (Non Mainnet)

- [x] I have considered running `./gradlew acceptanceTestNonMainnet`
locally if my PR affects non-mainnet modules.

## Changelog

- [x] I thought about the changelog and included a [changelog update if
required](https://wiki.hyperledger.org/display/BESU/Changelog).

---------

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
…rledger#5221)

<!-- Thanks for sending a pull request! Please check out our
contribution guidelines: -->
<!-- https://github.com/hyperledger/besu/blob/main/CONTRIBUTING.md -->

## PR description
This PR aims to remove the usage of getByBlockNumber from the
FlexiblePrivacyPrecompiledContract. As far as analysis shows the only
requirement is to get a protocolSpec (it doesn't matter which one)
currently it always fetches the protocolSpec for block number 1 which
can vary according to the network config. We're changing that get the
protocolSpec for block 0 which is the only block we always have the
header.

AT nonmainnet on circle CI
Pipeline - 21048

## Fixed Issue(s)
<!-- Please link to fixed issue(s) here using format: fixes #<issue
number> -->
<!-- Example: "fixes #2" -->
Fixes hyperledger#5160 

## Documentation

- [x] I thought about documentation and added the `doc-change-required`
label to this PR if
[updates are
required](https://wiki.hyperledger.org/display/BESU/Documentation).

## Acceptance Tests (Non Mainnet)

- [x] I have considered running `./gradlew acceptanceTestNonMainnet`
locally if my PR affects non-mainnet modules.

## Changelog

- [x] I thought about the changelog and included a [changelog update if
required](https://wiki.hyperledger.org/display/BESU/Changelog).

---------

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TeamGroot GH issues worked on by Groot Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate FlexiblePrivacyPrecompiledContract usage in RunnerBuilder.createPrivateTransactionObserver()
4 participants