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

Add EIP-5875: Instruction to Get Transaction Index of Block #5875

Closed
wants to merge 33 commits into from

Conversation

xinbenlv
Copy link
Contributor

@xinbenlv xinbenlv commented Nov 4, 2022

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@github-actions github-actions bot added c-new Creates a brand new proposal e-number Waiting on EIP Number assignment s-draft This EIP is a Draft labels Nov 4, 2022
@xinbenlv xinbenlv changed the title init eip txnum EIP-5875 Opcode for TX Number Nov 4, 2022
@xinbenlv xinbenlv marked this pull request as draft November 4, 2022 17:30
@eth-bot
Copy link
Collaborator

eth-bot commented Nov 4, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-5875.md

classification
newEIPFile

@github-actions github-actions bot added t-core and removed e-number Waiting on EIP Number assignment labels Nov 4, 2022
@xinbenlv xinbenlv marked this pull request as ready for review November 4, 2022 18:03
@frangio
Copy link
Contributor

frangio commented Nov 4, 2022

Previously discussed in #222.

@Pandapip1 Pandapip1 changed the title EIP-5875 Opcode for TX Number Add EIP-5875: Opcode for TX Number Nov 5, 2022
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Initial review. There's a lot to like!

EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
xinbenlv and others added 6 commits November 10, 2022 07:17
@xinbenlv
Copy link
Contributor Author

Thank you @Pandapip1 , all suggestions commited. Ready to merge

@lightclient
Copy link
Member

Also discussed here: #4363

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Generally:

  • s/tx number/transaction index/g
  • There are real implications of this in block production that need to be explored in the rationale and security considerations. For starters, how will this affect mev?

EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
xinbenlv and others added 2 commits November 11, 2022 12:20
Co-authored-by: lightclient <[email protected]>
Co-authored-by: lightclient <[email protected]>
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
1. Parameter Table

* `FORK_NUM_TBD`: the fork block number to start introducing this change.
* `OPCODE_TBD`: the opcode value being introduced.
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 unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I could remove it.

QQ: If we don't make OPCODE a placeholder number, how do we allow people to propose EIPs picking arbitrary unused new opcodes without conflicting with other drafts, knowing some of such core EIPs are taking long / never going to be adopted?


An example use-case is snapshot weighted voting, in which a snapshot of voting rights is generated. The current options are with `block.timesamp` or `block.number`, which only have block-level granularity. But the world state changes across transactions within a block. If there is any transaction in the same block that causes a voting right to change, the block number or block timestamp is not sufficient to pinpoint the world state of a snapshot of voting rights.

## Specification
Copy link
Member

Choose a reason for hiding this comment

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

You don't specify how much the operation costs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added now. Thank you!

EIPS/eip-5875.md Outdated

## Rationale

1. We choose the Transaction Index length for a world length for simplicity.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this sentence. Did you mean word length? In that case I would just omit this because the word length is not decidable by an instruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referring to the bounding. Updated the sentence and could you check if it reads better now?

EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
Co-authored-by: lightclient <[email protected]>
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
EIPS/eip-5875.md Outdated Show resolved Hide resolved
xinbenlv and others added 7 commits November 15, 2022 07:31
Co-authored-by: lightclient <[email protected]>
Co-authored-by: lightclient <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: lightclient <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: lightclient <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Comment on lines +52 to +54
4. Further discussion is needed about potentially bounding the transaction index for simpler on-chain computation if use case arises.
5. Further discussion is needed about whether to use transaction index within the block or globally. Globally makes it easier to compute number distance between two transactions directly but is useless for DAG
if we foresee a DAG future.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
4. Further discussion is needed about potentially bounding the transaction index for simpler on-chain computation if use case arises.
5. Further discussion is needed about whether to use transaction index within the block or globally. Globally makes it easier to compute number distance between two transactions directly but is useless for DAG
if we foresee a DAG future.
- Further discussion is needed about potentially bounding the transaction index for simpler on-chain computation if use case arises.
- Further discussion is needed about whether to use transaction index within the block or globally. Globally makes it easier to compute number distance between two transactions directly but is useless for DAG
if we foresee a DAG future.

@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Jan 10, 2023
@github-actions
Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Feb 21, 2023
Copy link

@Dawidowak Dawidowak left a comment

Choose a reason for hiding this comment

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

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-core w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants