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

EIP 2481: Adding request IDs to ETH protocol request and response objects #2481

Merged

Conversation

cburgdorf
Copy link
Contributor

No description provided.

@cburgdorf cburgdorf force-pushed the christoph/add-request-id-to-eth-protocol branch from cdb7fe6 to 0823cec Compare January 20, 2020 13:49
@cburgdorf cburgdorf force-pushed the christoph/add-request-id-to-eth-protocol branch 3 times, most recently from ec128d5 to d7fcfd9 Compare January 20, 2020 18:27
@cburgdorf cburgdorf force-pushed the christoph/add-request-id-to-eth-protocol branch from d7fcfd9 to 9eee53c Compare January 22, 2020 12:48
@cburgdorf cburgdorf force-pushed the christoph/add-request-id-to-eth-protocol branch from 9eee53c to 0cdf542 Compare January 22, 2020 12:49
@cburgdorf
Copy link
Contributor Author

Who do I need to bribe for this to get an EIP number assigned? Or should I just assign 2481 (the issue number) which seems to be used by convention?

@cburgdorf cburgdorf changed the title Add EIP arguing for request ids EIP 2481: Adding request IDs to ETH protocol request and response objects Apr 21, 2020
@cburgdorf
Copy link
Contributor Author

@karalabe I went wild west and have self assigned EIP number 2481 to this now (nobody assigned and EIP number to it so I went with the PR number which seems to be the way to go). Also Trinity has a draft PR for this now. It's not ready to merge but functional.

What would be the next steps here?

@karalabe
Copy link
Member

@cburgdorf You must implement it in Geth :trollface:

@cburgdorf
Copy link
Contributor Author

image

@axic
Copy link
Member

axic commented May 7, 2020

Or should I just assign 2481 (the issue number) which seems to be used by convention?

That is the convention, probably also explained in EIP-1?

<!--"If you can't explain it simply, you don't understand it well enough." Provide a simplified and layman-accessible explanation of the EIP.-->
This document proposes a way to increase the efficiency of the `eth` networking protocol while at the same time reducing complexity in Ethereum node implementations. It does so by introducing a request id to all requests which their corresponding responses must include.

## Abstract
Copy link
Member

Choose a reason for hiding this comment

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

This is quite a mouthful abstract 😉

@axic
Copy link
Member

axic commented May 7, 2020

@fjl @karalabe can this be merged as a draft now?

@karalabe
Copy link
Member

karalabe commented May 8, 2020

@axic I can't comment on the process requirements, but the content is something we've all agreed we should go forward with, so fine by me. Perhaps @cburgdorf, bring it up on next ACD call just so we have a "public debate" before going ahead with rolling it out. Just so that the EIP doesn't get passed behind closed doors.

@axic
Copy link
Member

axic commented May 8, 2020

The networking EIPs usually ignore "any process of EIPs", anyway 😉 Maybe also because the slow throughput of the EIP repo.

To me this looks ok as a draft. Ideally it would be in "last call" before complete rollout.

EIPS/eip-2481.md Outdated
## Security Considerations
<!--All EIPs must contain a section that discusses the security implications/considerations relevant to the proposed change. Include information that might be important for security discussions, surfaces risks and can be used throughout the life cycle of the proposal. E.g. include security-relevant design decisions, concerns, important discussions, implementation-specific guidance and pitfalls, an outline of threats and risks and how they are being addressed. EIP submissions missing the "Security Considerations" section will be rejected. An EIP cannot proceed to status "Final" without a Security Considerations discussion deemed sufficient by the reviewers.-->

None?
Copy link
Member

Choose a reason for hiding this comment

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

I would it shouldn't go "final" with a question mark.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I was shy to deny security implications entirely but the EIP has had some 👀 on it by now and nobody has brought up any concerns. I just removed the question mark now.

@cburgdorf
Copy link
Contributor Author

@karalabe @axic I've proposed this for the next ACD call on Friday and will attend the meeting to present it.

@axic
Copy link
Member

axic commented May 12, 2020

Lets merge this as a proper draft.

@Souptacular Souptacular merged commit 9ba544f into ethereum:master May 15, 2020
pizzarob pushed a commit to pizzarob/EIPs that referenced this pull request Jun 12, 2020
…ects (ethereum#2481)

* Add EIP arguing for request ids

* Assign EIP number 2481

* Add link to draft implementation in Trinity

* Fix spelling issues

* Change file name

* Remove question mark from security considerations
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
…ects (ethereum#2481)

* Add EIP arguing for request ids

* Assign EIP number 2481

* Add link to draft implementation in Trinity

* Fix spelling issues

* Change file name

* Remove question mark from security considerations
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
…ects (ethereum#2481)

* Add EIP arguing for request ids

* Assign EIP number 2481

* Add link to draft implementation in Trinity

* Fix spelling issues

* Change file name

* Remove question mark from security considerations
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.

5 participants