rpc: change BlockNumber constant values to match ethclient #27219

holiman · 2023-05-04T08:40:21Z

An off-comment here https://github.com/ethereum/go-ethereum/pull/21177/files/07d5cfbf0a5e92a51459a43fe4021ceccae428b3#diff-1053ae80885a491cefa0ab360c605e7abbcec41fbb620c070a132c58a3bd6cac , caused an error: #27205 .
This PR fixes that error

ethclient/ethclient.go

panicalways · 2023-05-04T10:02:34Z

Isn't that a big breaking change?

// Get pending balance
ethclient.BalanceAt(..., big.NewInt(-1))

Anyone who was using it as above will be broken.

holiman · 2023-05-04T10:08:37Z

Meh, maybe..? Although, anyone depending on 'pending' to be meaningfully different from 'latest' is already working under flawed premises. Pending is a very vague concept.

s1na

LGTM. We should add a backwards incompatibility note in the release

karalabe · 2023-05-04T15:05:10Z

Using negative numbers for this is so so so bad. Still, shouldn't the negative numbers only be some internal implementation detail? They should not be exposed to the outside world afaik. cc @fjl?

rjl493456442 · 2023-05-05T07:54:32Z

I agree we shouldn't expose internal implementation specific detail to users. We need to change the block parameter type to a compound one which can be either a valid block number or a default block params(finalized, pending, etc).

But it requires a breaking change.

fjl · 2023-05-05T17:22:03Z

I hate that we have support for these special negative numbers in the first place. The only reason we have them is because I gave in on some PR years ago.

In the original ethclient API, we had support for "latest" by passing a nil block number as the only special case. This was at the time when block numbers were *big.Int everywhere, so the choice of nil seemed quite reasonable. We didn't have explicit support for "pending". It's not often that people need to get the 'pending header'.

ethclient does have an explicit way of accessing pending state, with special methods.

We added support for -1 being "pending" in #21177. Initially, it was implemented such that all negative numbers would mean "pending". The use case this was added for, is specifying the block ranges for log filtering. Note that we didn't think about rpc.BlockNumber compatibility when this feature was added. We just needed a way to add support for this special value and nil was already taken.

Later, we added support for "safe" and "finalized" in the same way, in #25580. These do make sense for BlockByNumber, etc. It was obvious to add them using the same numbering system used by rpc.BlockNumber because the new tags had just been added there as well, and we had to find some way of putting them in.

fjl · 2023-05-05T17:26:33Z

The problem is, -1 is part of the ethclient API, and has been for quite a while.

If we have to make a change at all, I would suggest we change the definition of rpc.PendingBlockNumber to be -1 and define rpc.LatestBlockNumber as -2. The specific values of these constants are not important, and they are not otherwise exposed in public APIs.

holiman · 2023-05-09T11:56:48Z

If we have to make a change at all, I would suggest we change the definition of rpc.PendingBlockNumber to be -1 and define rpc.LatestBlockNumber as -2. The specific values of these constants are not important, and they are not otherwise exposed in public APIs.

I don't see the point of that. If we do like in this PR, then:

How we break backwards-compatilibity: caller previously getting pending (via -1) is now getting latest instead.

With your proposed change,

How we break backwards-compatilibity: caller previously getting latest (via -1) is now getting pending instead.

I think we should move towards minimizing the use of pending, since the concept is so vague.

and they are not otherwise exposed in public APIs.

Are you sure?
Anyway, maybe I misunderstand something here. Perhaps better to discuss

fjl · 2023-05-09T22:45:43Z

I meant, we could swap the values of the constants instead of applying changes in this PR. Then ethclient's definition of -1 == "pending" would be the same as rpc.PendingBlockNumber.

There is no other code in go-ethereum that relies on the numeric value of these constants. rpc.BlockNumber is really only used as a parameter type in RPC method handlers, and so will always be decoded from JSON. Passing a negative block number is not allowed in JSON, the decoder will error (see https://github.com/ethereum/go-ethereum/blob/master/rpc/types.go#L79). You can only get these negative values by passing a block specifier string such as "pending".

holiman · 2023-05-10T14:25:52Z

@fjl PTAL

fjl · 2023-05-10T17:47:33Z

Interesting, a filters-related test failed. Need to investigate this.

…tate

- avoid creating big.Int just to compare with BlockNumber - use String method of BlockNumber

s1na · 2023-05-23T09:05:52Z

eth/filters/api_test.go

@@ -56,7 +56,8 @@ func TestUnmarshalJSONNewFilterArgs(t *testing.T) {

 	// from, to block number
 	var test1 FilterCriteria
-	vector := fmt.Sprintf(`{"fromBlock":"%#x","toBlock":"%#x"}`, fromBlock, toBlock)
+	vector := fmt.Sprintf(`{"fromBlock":"%v","toBlock":"%v"}`, fromBlock, toBlock)
+	t.Logf("vector: %s", vector)


Seems like debugging debris

…27219) ethclient accepts certain negative block number values as specifiers for the "pending", "safe" and "finalized" block. In case of "pending", the value accepted by ethclient (-1) did not match rpc.PendingBlockNumber (-2). This wasn't really a problem, but other values accepted by ethclient did match the definitions in package rpc, and it's weird to have this one special case where they don't. To fix it, we decided to change the values of the constants rather than changing ethclient. The constant values are not otherwise significant. This is a breaking API change, but we believe not a dangerous one. --------- Co-authored-by: Felix Lange <fjl@twurst.com>

…thereum#27219)" This reverts commit c27eb12.

rjl493456442 reviewed May 4, 2023

View reviewed changes

ethclient/ethclient.go Outdated Show resolved Hide resolved

ethclient: fix discrepancy between latest/pending blocknumbers

562be5f

holiman force-pushed the fix_ethclient branch from 8054de4 to 562be5f Compare May 4, 2023 11:22

rjl493456442 approved these changes May 4, 2023

View reviewed changes

s1na approved these changes May 4, 2023

View reviewed changes

holiman added the status:triage label May 9, 2023

rpc: swap rpc.Latest and rpc.Pending block number (internal) constant

11ad739

holiman requested a review from fjl as a code owner May 10, 2023 14:25

eth/filters: use constants instead of magic, handle missing pending s…

d64678e

…tate

holiman requested a review from karalabe as a code owner May 11, 2023 09:01

fjl added 3 commits May 16, 2023 16:37

rpc: add String method for BlockNumber

3ea8e2b

ethclient: improve toBlockNumArg

236358e

- avoid creating big.Int just to compare with BlockNumber - use String method of BlockNumber

ethclient/gethclient: copy new toBlockNumArg

c0cf2ac

tynes mentioned this pull request May 18, 2023

op-e2e: pending block test + op-geth update ethereum-optimism/optimism#5736

Merged

fjl changed the title ~~ethclient: fix discrepancy between latest/pending blocknumbers~~ rpc: change BlockNumber constant values to match ethclient May 23, 2023

eth/filters: fix test

48f68ab

fjl approved these changes May 23, 2023

View reviewed changes

s1na reviewed May 23, 2023

View reviewed changes

eth/filters: remove log

025270c

s1na mentioned this pull request May 23, 2023

internal/ethapi: add more testcase for block/header rpc #27325

Merged

fjl removed the status:triage label May 23, 2023

fjl merged commit 9231770 into ethereum:master May 23, 2023

fjl added this to the 1.11.7 milestone May 23, 2023

s1na mentioned this pull request May 23, 2023

PendingBlockNumber has different values across modules #27205

Closed

holiman deleted the fix_ethclient branch October 11, 2023 07:27

0xcb9ff9 mentioned this pull request Oct 30, 2023

Upstream PR backporting dogechain-lab/dbsc#43

Open

3 tasks

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "rpc: change BlockNumber constant values to match ethclient (e…

01a2c58

…thereum#27219)" This reverts commit c27eb12.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "rpc: change BlockNumber constant values to match ethclient (e…

4aa95f2

…thereum#27219)" This reverts commit c27eb12.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: change BlockNumber constant values to match ethclient #27219

rpc: change BlockNumber constant values to match ethclient #27219

holiman commented May 4, 2023

panicalways commented May 4, 2023

holiman commented May 4, 2023 via email

s1na left a comment

karalabe commented May 4, 2023

rjl493456442 commented May 5, 2023

fjl commented May 5, 2023 •

edited

Loading

fjl commented May 5, 2023 •

edited

Loading

holiman commented May 9, 2023

fjl commented May 9, 2023

holiman commented May 10, 2023

fjl commented May 10, 2023

s1na May 23, 2023

rpc: change BlockNumber constant values to match ethclient #27219

rpc: change BlockNumber constant values to match ethclient #27219

Conversation

holiman commented May 4, 2023

panicalways commented May 4, 2023

holiman commented May 4, 2023 via email

s1na left a comment

Choose a reason for hiding this comment

karalabe commented May 4, 2023

rjl493456442 commented May 5, 2023

fjl commented May 5, 2023 • edited Loading

fjl commented May 5, 2023 • edited Loading

holiman commented May 9, 2023

fjl commented May 9, 2023

holiman commented May 10, 2023

fjl commented May 10, 2023

s1na May 23, 2023

Choose a reason for hiding this comment

fjl commented May 5, 2023 •

edited

Loading

fjl commented May 5, 2023 •

edited

Loading