5 ms·

It's not fixed yet, the OE instance in forkmon was syncing the whole time and hasn't hit the issue yet.

by fjl 5y ago

arberx 5y ago
You sure? Thought it was in a failed state earlier? I think a block recently processed on OE, that's why it's showing as good. But yeah, don't think it's fixed yet.
- fjl 5y ago
  The fix is in https://github.com/openethereum/openethereum/pull/364 https://github.com/openethereum/openethereum/pull/364 Actually, it's this commit of the PR: https://github.com/openethereum/openethereum/pull/364/commits/a49cc34d1ce353b2ab7d271dad70e8d81d5f11a3 https://github.com/openethereum/openethereum/pull/364/commit...
  - runeks 5y ago
    386 files changed? This has to include more than just the fix, right?
    - deleted 5y ago
      [deleted]
    - warp 5y ago
      Looks like they've extracted it to a separate hotfix PR here: https://github.com/openethereum/openethereum/pull/366 https://github.com/openethereum/openethereum/pull/366
      - xiphias2 5y ago
        Shouldn't at least a unit test be added before accepting a pull request? We're talking about 15% of a 300 billion dollar currency. I have written tests for smaller things than that.
        AgentME 5y ago
        If it's true that Ethereum blocks have any kind of checksum of the expected results, then if the patch has a mistake, I'd think the failure mode would just be that they continue to fail processing the failed block, or they get further but fail to process a more recent block. In that case, then I think it would make sense here to rush out a patch that's been manually checked, and then go back to add some tests.
        xiphias2 5y ago
        The right way to fix this is to have an integration test that passes on all other implementations but fails on OpenEthereum, and push this fix afterwards. People are storing and trading billions of dollars on Ethereum, so I think both automated and manual checking is important. Also an integration test for all branches should be able to find these errors.
  - makomk 5y ago
    Looks like some really subtle and (as far as I can tell) undocumented aspect of Ethereum internals tripped them up.
    - AgentME 5y ago
      They didn't properly implement EIP-2929 (https://eips.ethereum.org/EIPS/eip-2929 https://eips.ethereum.org/EIPS/eip-2929). I looked at the first section of the code diff in the PR and it matched up with stuff already documented in the EIP, so I wouldn't assume the issue was because of undocumented stuff. Multiple other Ethereum implementations managed to implement the EIP-2929 spec correctly.

It's not fixed yet, the OE instance in forkmon was syncing the whole time and hasn't hit the issue yet.

by fjl 5y ago

It's not fixed yet, the OE instance in forkmon was syncing the whole time and hasn't hit the issue yet.

You sure? Thought it was in a failed state earlier? I think a block recently processed on OE, that's why it's showing as good. But yeah, don't think it's fixed yet.

fjl 5y ago
The fix is in https://github.com/openethereum/openethereum/pull/364 https://github.com/openethereum/openethereum/pull/364 Actually, it's this commit of the PR: https://github.com/openethereum/openethereum/pull/364/commits/a49cc34d1ce353b2ab7d271dad70e8d81d5f11a3 https://github.com/openethereum/openethereum/pull/364/commit...
- runeks 5y ago
  386 files changed? This has to include more than just the fix, right?
  - deleted 5y ago
    [deleted]
  - warp 5y ago
    Looks like they've extracted it to a separate hotfix PR here: https://github.com/openethereum/openethereum/pull/366 https://github.com/openethereum/openethereum/pull/366
    - xiphias2 5y ago
      Shouldn't at least a unit test be added before accepting a pull request? We're talking about 15% of a 300 billion dollar currency. I have written tests for smaller things than that.
      - AgentME 5y ago
        If it's true that Ethereum blocks have any kind of checksum of the expected results, then if the patch has a mistake, I'd think the failure mode would just be that they continue to fail processing the failed block, or they get further but fail to process a more recent block. In that case, then I think it would make sense here to rush out a patch that's been manually checked, and then go back to add some tests.
        xiphias2 5y ago
        The right way to fix this is to have an integration test that passes on all other implementations but fails on OpenEthereum, and push this fix afterwards. People are storing and trading billions of dollars on Ethereum, so I think both automated and manual checking is important. Also an integration test for all branches should be able to find these errors.
- makomk 5y ago
  Looks like some really subtle and (as far as I can tell) undocumented aspect of Ethereum internals tripped them up.
  - AgentME 5y ago
    They didn't properly implement EIP-2929 (https://eips.ethereum.org/EIPS/eip-2929 https://eips.ethereum.org/EIPS/eip-2929). I looked at the first section of the code diff in the PR and it matched up with stuff already documented in the EIP, so I wouldn't assume the issue was because of undocumented stuff. Multiple other Ethereum implementations managed to implement the EIP-2929 spec correctly.