Skip to content

eth_feeHistory#2432

Closed
RatanRSur wants to merge 39 commits intobesu-eth:masterfrom
RatanRSur:fee-history
Closed

eth_feeHistory#2432
RatanRSur wants to merge 39 commits intobesu-eth:masterfrom
RatanRSur:fee-history

Conversation

@RatanRSur
Copy link
Copy Markdown
Contributor

@RatanRSur RatanRSur commented Jun 14, 2021 •
edited
Loading

The spec for this implementation is listed in the issue below.

Fixed Issue(s)

fixes #2430

Changelog

@timbeiko timbeiko mentioned this pull request Jun 15, 2021
Merged
RatanRSur added 17 commits June 16, 2021 21:30
@RatanRSur
beginning
62dd31f 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
fee history class and other stuff
ec78644 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
get first 3 fields working
27a79f1 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
add a bunch of stuff to the failing test
e250c30 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
add headers
22dd003 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
spotless
dedf4dc 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
get all fields test working
b5a6470 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
0 for base fee
3f7ac5f 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
add extra base fee
97a147c 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
update names to latest spec
1470c40 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
test block count higher than chain head
7f61250 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
add test for all zeros
70fdcca 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
error on block too high
15ce6b5 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
spotless
8b3a7e0 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
get rpc spec test wired up and failing
4a4a354 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
fix tests
ad89e50 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
spotless
d0051dc 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
RatanRSur added 4 commits June 16, 2021 21:41
@RatanRSur
sorting and comments
60b5184 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
remove redundant assignment
c55e7a9 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
fix transaction selector
760338f 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
make test presume 1559
b169d79 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur RatanRSur marked this pull request as ready for review June 17, 2021 03:37
@RatanRSur RatanRSur changed the title fee history eth_feeHistory Jun 17, 2021
matkt
matkt requested changes Jun 17, 2021
View reviewed changes
Comment thread .../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistory.java
Comment thread .../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistory.java Outdated
Comment thread .../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistory.java Outdated
RatanRSur added 2 commits June 17, 2021 09:57
@RatanRSur
fix range errors
cfb06bc 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
switch to Immutable
253259f 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
garyschulte
garyschulte requested changes Jun 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

suggestions re: arithmetic and input validation

Comment thread .../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistory.java
Comment thread .../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistory.java
Comment thread .../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistory.java
Comment thread .../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistory.java Outdated
Comment thread .../test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthFeeHistoryTest.java
RatanRSur added 2 commits June 17, 2021 14:32
@RatanRSur
test upper bound for block count
8aa77a6 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
cleanup
4aa9cc5 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
garyschulte
garyschulte requested changes Jun 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

LGTM, just need to address effectivePriorityFee for legacy transactions post-london

Comment thread ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java Outdated
garyschulte
garyschulte approved these changes Jun 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@garyschulte garyschulte left a comment •
edited
Loading

Choose a reason for hiding this comment

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

🚢 (after unit tests)

garyschulte
garyschulte requested changes Jun 18, 2021
View reviewed changes
Comment thread ...creation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java Outdated
@RatanRSur RatanRSur enabled auto-merge (squash) June 21, 2021 20:24
garyschulte
garyschulte requested changes Jun 21, 2021
View reviewed changes
Comment thread ethereum/core/src/test-support/java/org/hyperledger/besu/ethereum/core/BlockDataGenerator.java
private TransactionType[] transactionTypes = TransactionType.values();
private Optional<Address> coinbase = Optional.empty();
private Optional<Long> baseFee = Optional.empty();
private Optional<Optional<Long>> maybeBaseFee = Optional.empty();
Copy link
Copy Markdown
Contributor

@garyschulte garyschulte Jun 21, 2021

Choose a reason for hiding this comment

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

Optional<Optional<Long>> ?

Copy link
Copy Markdown
Contributor Author

@RatanRSur RatanRSur Jun 22, 2021

Choose a reason for hiding this comment

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

Yeah because we need a way to encode if the basefee is unspecified. This is important for knowing whether to use a default base fee or the optional.empty the caller gives explicitly to the builder.

Comment thread ...reum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/JsonRpcHttpServiceTest.java Outdated
@RatanRSur
fix epfpg calc
b07979c 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@matkt
Copy link
Copy Markdown
Contributor

matkt commented Jun 22, 2021

I'm just concerned about the performance of this API when blockcount == 500 on calaveras. I wanted to try and after 2 minutes and several thread blocked we have the result. I also wanted to test with a blockcount == 10 starting from the block 6400 and I had 23 seconds. These blocks are filled by transactions

What is the blockcount requested in the common case ?

@RatanRSur
Copy link
Copy Markdown
Contributor Author

RatanRSur commented Jun 22, 2021

I think block counts of 500 would be very unusual but 10 sounds reasonable.

RatanRSur added 3 commits June 22, 2021 09:53
@RatanRSur
fix lambda names
89cdd45 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
remove unnecessary check
a65895d 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur
changelog
47de7f9 
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@garyschulte
Copy link
Copy Markdown
Contributor

garyschulte commented Jun 22, 2021

I think block counts of 500 would be very unusual but 10 sounds reasonable.

How about we silently map anything >10 to 10. We might fail some test cases in hive, but this seems like an acceptable tradeoff for now.

garyschulte
garyschulte approved these changes Jun 22, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

🚢

@RatanRSur RatanRSur closed this Jun 22, 2021
auto-merge was automatically disabled June 22, 2021 18:13

Pull request was closed

@garyschulte garyschulte added the documentation Improvements or additions to documentation label Jun 24, 2021
@bgravenorst bgravenorst mentioned this pull request Jul 16, 2021
Closed
@rolandtyler rolandtyler removed the documentation Improvements or additions to documentation label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matkt matkt matkt requested changes

@garyschulte garyschulte garyschulte approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add feeHistory JSON RPC API

4 participants

@RatanRSur @matkt @garyschulte @rolandtyler