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

feat(api/c-chain): change of behavior for eth_getProof #1996

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Jan 13, 2025

No description provided.

Copy link

vercel bot commented Jan 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
avalanche-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 9:45am
builders-hub ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 9:45am

Note the `eth_getProof` is acting differently from release v0.14.1:

- On archive nodes (`"pruning-enabled": false`): queries for historical proofs for state older than approximately 24 hours preceding the last accepted block will be rejected by default. This can be adjusted with the new option `historical-proof-query-window` which defines the number of blocks before the last accepted block which should be accepted for state proof queries, or set to `0` to accept any block number state query.
- On `pruning` nodes: queries for proofs past the tip buffer (32 blocks) will be rejected
Copy link
Contributor Author

@qdm12 qdm12 Jan 13, 2025

Choose a reason for hiding this comment

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

this is the same (except the error message I suppose) as the previous behavior isn't it? 🤔
If it is, should we remove it from here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the exact same behavior as they would have responded to proofs for blocks matching the commit height 4096, so we can leave this here.

Also perhaps this should be under the Ethereum APIs section, we can add a new section for eth_getProof if there's not one already and just include this notice, mentioning the API otherwise behaves as upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already in the ## Ethereum APIs section / ### Standard Ethereum APIs sub-section.
I've moved this to an #### Exceptions subsection as a bullet point, let me know if that works.

Copy link
Collaborator

@meaghanfitzgerald meaghanfitzgerald left a comment

Choose a reason for hiding this comment

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

suggestions to improve readability. also removes extra blank line to please the .md linter

Comment on lines +86 to +91
- `eth_getProof` behaves differently than geth, from release v0.14.1, with the following differences:
- On archive nodes (`"pruning-enabled": false`), queries for proofs for state older than approximately 24 hours preceeding the last accepted block will be rejected by default.
This can be adjusted with the option `historical-proof-query-window` which defines the number of blocks before the last accepted block which should be accepted for state proof queries, or set it to `0` to accept any block number state query.
- On pruning nodes (`"pruning-enabled": true`), queries for proofs past the tip buffer of 32 blocks are always rejected


Copy link
Collaborator

@meaghanfitzgerald meaghanfitzgerald Jan 14, 2025

Choose a reason for hiding this comment

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

Suggested change
- `eth_getProof` behaves differently than geth, from release v0.14.1, with the following differences:
- On archive nodes (`"pruning-enabled": false`), queries for proofs for state older than approximately 24 hours preceeding the last accepted block will be rejected by default.
This can be adjusted with the option `historical-proof-query-window` which defines the number of blocks before the last accepted block which should be accepted for state proof queries, or set it to `0` to accept any block number state query.
- On pruning nodes (`"pruning-enabled": true`), queries for proofs past the tip buffer of 32 blocks are always rejected
`eth_getProof` behaves differently than geth, from release `v0.14.1`, with the following differences:
- On archival nodes (nodes with`pruning-enabled` set to `false`), queries for state proofs older than 24 hours preceding the last accepted block will be rejected by default. This can be adjusted with `historical-proof-query-window`, which defines the number of blocks before the last accepted block that can be queried for state proofs. Set this option to `0` to accept a state query for any block number.
- On pruned nodes (nodes with `pruning-enabled` set to `true`), queries for state proofs past a 32 block buffer are always rejected.

Copy link
Contributor Author

@qdm12 qdm12 Jan 15, 2025

Choose a reason for hiding this comment

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

  1. TIL I've been using preceeding wrong forever, thanks! Only 1 e!
  2. Sorry for the extra line, I hate those extra lines as much as the md linter!
  3. Isn't it pruning nodes instead of pruned nodes? We're talking about avalanche nodes, not i.e. trie nodes 🤔
  4. queries for state proofs past a 32 block buffer are always rejected. ➡️ queries for state proofs outside the 32 blocks window trailing the last accepted block are always rejected. - since the 32 block buffer might leave some room for interpretation and is a bit of an implementation details I'd think?

Thanks!

@@ -81,6 +81,14 @@ For batched requests on the [public API node](/tooling/rpc-providers) , the maxi
number of items is 40. We are working on to support a larger batch size.
</Callout>

#### Exceptions

- `eth_getProof` behaves differently than geth, from release v0.14.1, with the following differences:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the release version should the avalanchego version, not the coreth version. I suppose we should wait for the next avalanchego release using coreth v0.14.1 (unreleased) to merge this PR.

@qdm12 qdm12 added the DO NOT MERGE This PR is not meant to be merged in its current state label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE This PR is not meant to be merged in its current state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants