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

Logs: Indexing/Filtering bugs #1345

Closed
Stebalien opened this issue Dec 20, 2022 · 9 comments
Closed

Logs: Indexing/Filtering bugs #1345

Stebalien opened this issue Dec 20, 2022 · 9 comments

Comments

@Stebalien
Copy link
Member

  1. It looks like we're decoding the value before inserting it, assuming that the value is a cbor-encoded byte array. We should insert the CBOR as-is, and specify CBOR objects in the filters. See https://github.com/filecoin-project/lotus/blob/f4cc5541e4570194e8abdc043964aafdb01f9975/chain/events/filter/index.go#L198
  2. We shouldn't have any "trim leading zeros" logic in the event system (https://github.com/filecoin-project/lotus/blob/f4cc5541e4570194e8abdc043964aafdb01f9975/chain/events/filter/index.go#L276). This is an Ethereum API thing. Instead:
    1. We need to trim leading zeros in the ethereum API.
    2. Then cbor-encode as a cbor "bytes".
  3. As of f4cc5541e4570194e8abdc043964aafdb01f9975, https://github.com/filecoin-project/lotus/blob/f4cc5541e4570194e8abdc043964aafdb01f9975/chain/events/filter/index_test.go#L18 is broken.
@raulk
Copy link
Member

raulk commented Jan 13, 2023

(3) is fixed. (1) and (2) will be addressed later.

@maciejwitowski
Copy link
Contributor

@raulk could @iand handle this?

@maciejwitowski
Copy link
Contributor

Will move to Ian after changes in FIP49

@iand
Copy link

iand commented Jan 18, 2023

1 will invalidate existing data in database. Not a big deal at this time but thought I would note it here.

@raulk
Copy link
Member

raulk commented Jan 19, 2023

@iand that's fine, node operators are going to be resetting their chain and resyncing from Hyperspace genesis to fix other issues, so this is the right time to introduce any breaking changes to the events index.

@iand
Copy link

iand commented Jan 20, 2023

While adding tests I've encountered various places where values are being compared incorrectly (cbor encoded against raw) or double decoded. I'm fixing them all to so we are consistent and tests pass. Currently the consistent position is that all filters and database entries use raw values and test against cbor-decoded event logs. Once we have the improved test coverage I will change that convention as described in this issue: all internal filters and database entries will use cbor-encoded values and we will encode/decode in the API.

@iand
Copy link

iand commented Jan 20, 2023

filecoin-project/lotus#10085 rationalises the cbor encoding hacks and fixed the broken test noted in 3 as a consequence

@iand
Copy link

iand commented Jan 24, 2023

filecoin-project/lotus#10085 was merged into filecoin-project/lotus#10083 so this should be fixed when that lands

@iand
Copy link

iand commented Jan 26, 2023

filecoin-project/lotus#10083 was merged to 1.20.0 so this should be closed as done

@github-project-automation github-project-automation bot moved this to ✅ Done in Network v18 Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants