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

internal/ethapi: remove header.Size in rpc getHeaderByXXX #27347

Merged
merged 1 commit into from
May 25, 2023

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented May 25, 2023

As discussed with @rjl493456442 and also in #27325 (comment). Currently, the PRC of eth_getHeaderByXXX's response will return the header's Size, which depends on the struct Header's size:

var headerSize = common.StorageSize(reflect.TypeOf(Header{}).Size())
// Size returns the approximate memory used by all internal contents. It is used
// to approximate and limit the memory consumption of various caches.
func (h *Header) Size() common.StorageSize {
var baseFeeBits int
if h.BaseFee != nil {
baseFeeBits = h.BaseFee.BitLen()
}
return headerSize + common.StorageSize(len(h.Extra)+(h.Difficulty.BitLen()+h.Number.BitLen()+baseFeeBits)/8)
}

And this struct's size differs in the different distros(x32 VS x64), so I proposed to remove this field for:

  1. getHeaderByXXX is Geth's own RPC, not in standard ones;
  2. We have block's size(Guaranteed by consensus), someone may be confused about those two.

Signed-off-by: jsvisa <delweng@gmail.com>
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

I mostly agree with dropping this, but I am unsure why we even have it in the first place. It has "always" been there, and as far as I can tell it has "always" been wrong, using usafe.sizeOf and thus obtaining a platform-dependent value for size.

IMO we should either return e.g. the rlp-encoded size or nothing at all. But IMO needs some more backstory.

@s1na
Copy link
Contributor

s1na commented May 25, 2023

getHeaderByXXX is Geth's own RPC, not in standard ones;

Interesting, I thought our "eth" methods match the specs with the exception of eth_call taking extra parameters. Seems you're right, getHeaderBy* are indeed not part of the specs. We should at least document them on our website and possibly move them to another namespace as part of this re-structure

@s1na
Copy link
Contributor

s1na commented May 25, 2023

Judging from Header.Size comment:

It is used to approximate and limit the memory consumption of various caches.

It shouldn't be returned from API.

@jsvisa
Copy link
Contributor Author

jsvisa commented May 25, 2023

but I am unsure why we even have it in the first place.

I know what's going on, this field is changed in 530f78e?diff=unified#diff-c426ecd2f7d247753b9ea8c1cc003f21fa412661c1f967d203d4edf8163da344L939-R980

And the old code is correct, equals to block.Size()

image

@jsvisa
Copy link
Contributor Author

jsvisa commented May 25, 2023

We have several ways to do this:

  1. keep with the old style, return to the block's size
  2. return the header's RLP size
  3. just remove this field

My personal preference 3 > 2 > 1

@s1na
Copy link
Contributor

s1na commented May 25, 2023

I think 2 doesn't make sense. Since the header has mostly information about the block, not itself. I'm slightly in favor of 1 than 3. We have the field already and block size could possibly be useful.

@holiman
Copy link
Contributor

holiman commented May 25, 2023

I know what's going on, this field is changed in 530f78e?diff=unified#diff-c426ecd2f7d247753b9ea8c1cc003f21fa412661c1f967d203d4edf8163da344L939-R980

And the old code is correct, equals to block.Size()

Nope, that commit added the RPCMarshalHeader. The old RPCMarshalBlock still uses the b.Size():

https://github.com/ethereum/go-ethereum/pull/19669/files#diff-c426ecd2f7d247753b9ea8c1cc003f21fa412661c1f967d203d4edf8163da344R994

So RPCMarshalHeader has always been wrong. And in that case, I think we can just remove it.

@jsvisa
Copy link
Contributor Author

jsvisa commented May 25, 2023

I'm slightly in favor of 1 than 3. We have the field already and block size could possibly be useful.

IMHO, If someone is interested in the size of the block, he has to retrieve it via eth_getBlockByXXX instead of this one. And BTW, if we have to return the block's size, we need to retrieve the block from the backend either, probably not a good deal.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM

@s1na s1na merged commit 1816cdc into ethereum:master May 25, 2023
@s1na s1na added this to the 1.12.1 milestone May 25, 2023
@jsvisa jsvisa deleted the internal-ethapi-drop-header-size branch May 25, 2023 13:58
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
RPC methods `eth_getHeaderBy*` returned a size value which was meant for internal
processes. Please instead use `size` field returned by `eth_getBlockBy*` if you're interested
in the RLP encoded storage size of the block.

Signed-off-by: jsvisa <delweng@gmail.com>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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.

3 participants