Skip to content
This repository has been archived by the owner on Jan 22, 2019. It is now read-only.

Implement EIP-234's get logs by blockHash #38

Open
nionis opened this issue Aug 2, 2018 · 22 comments
Open

Implement EIP-234's get logs by blockHash #38

nionis opened this issue Aug 2, 2018 · 22 comments
Labels
gitcoin There's a Gitcoin bounty attached to this! good first issue Good for newcomers

Comments

@nionis
Copy link
Contributor

nionis commented Aug 2, 2018 •
edited by shrugs
Loading

Issue

Currently we use eth_getLogs to request logs per block, but this endpoint only responds to block numbers, which means it's possible to get the wrong logs during a fork 😨

Solution

So, once parity and geth ship the ability to get logs by block hash, we can support that as the default behavior in gnarly, and update the various reducers that import getLogs from gnarly-core.

(edited by @shrugs)
(original, unedited issue:)

EIP-234

This will help with state consistency, you can get more details here
@mkosowsk
Copy link

mkosowsk commented Aug 7, 2018

Hi @nionis, just to confirm that

Adds a blockHash option to params field of eth_getLogs JSON-RPC call.

Returns logs matching filter criteria only for all transactions in the block with hash blockHash. This works the same as if you specify fromBlock= toBlock = (# of block whose hash is blockHash), but allows the call to work if the RPC client knows only the block hash but not the block number.

Returns invalid param error if both blockHash and either fromBlock or toBlock are specified.

if this criteria is met with a PR, then that resolves this issue? Thanks! 👍

@nionis
Copy link
Contributor Author

nionis commented Aug 7, 2018 •
edited
Loading

@mkosowsk Yes but before updating reducer's getLogs we need to make sure both geth and parity have implemented EIP-234, right now its available on geth, but not parity.

Once that is done then we can update all reducers and make it the default behaviour.
On Gnarly we only need to update reducers
parity

@mkosowsk
Copy link

mkosowsk commented Aug 7, 2018

Great, thanks!

@shrugs shrugs added good first issue Good for newcomers gitcoin There's a Gitcoin bounty attached to this! labels Aug 7, 2018
@shrugs
Copy link
Member

shrugs commented Aug 7, 2018

I've edited the issue to explain the background and solution requirements

@mkosowsk
Copy link

mkosowsk commented Aug 7, 2018

@shrugs thanks!

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 225.0 DAI (225.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot
Copy link

gitcoinbot commented Aug 10, 2018 •
edited
Loading

Issue Status: 1. Open 2. Cancelled

Work has been started.

These users each claimed they can complete the work by 10 months, 3 weeks from now.
Please review their action plans below:

1) nionis has been approved to start work.

1.Make sure EIP-234 is implemented on:

  • geth
  • parity
  • ganache
  1. Then implement getLogs by hash on all reducers

Learn more on the Gitcoin Issue Details page.

@gitcoinbot
Copy link

@nionis Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@nionis
Copy link
Contributor Author

nionis commented Aug 13, 2018

Parity merged EIP to master, waiting for release
and also ganache

@gitcoinbot
Copy link

@nionis Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@nionis
Copy link
Contributor Author

nionis commented Aug 21, 2018

@gitcoinbot
Parity merged EIP to master, waiting for release
and also waiting ganache implementation

@gitcoinbot
Copy link

@nionis Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@vs77bb
Copy link

vs77bb commented Aug 26, 2018

Sorry about @gitcoinbot, @nionis - snoozing for 10 days!

@nionis
Copy link
Contributor Author

nionis commented Aug 31, 2018

@shrugs parity release is available!
should we wait also for ganache implementation?

@shrugs
Copy link
Member

shrugs commented Aug 31, 2018

Good question. Probably best to wait for it, before merging, unfortunately. I suppose the implementation can be tested on parity and geth beforehand and primed for merge, though.

@gitcoinbot
Copy link

@nionis Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled

The funding of 225.0 DAI (225.0 USD @ $1.0/DAI) attached to this issue has been cancelled by the bounty submitter

@gitcoinbot
Copy link

@nionis Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@nionis due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@shrugs
Copy link
Member

shrugs commented Sep 17, 2018

(for context, gnarly development has been paused/discontinued and gitcoin knows, so we can ignore these messages)

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@nionis due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled

The funding of 225.0 DAI (225.0 USD @ $1.0/DAI) attached to this issue has been cancelled by the bounty submitter

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
gitcoin There's a Gitcoin bounty attached to this! good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@shrugs @nionis @mkosowsk @vs77bb @gitcoinbot