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

itests: add event matrix tests for realtime eth filters and subscriptions #10083

Merged
merged 5 commits into from
Jan 26, 2023

Conversation

iand
Copy link
Contributor

@iand iand commented Jan 20, 2023

Related Issues

Part of #9849

Proposed Changes

Bugs fixed:

  • realtime filters/subs were using old method of checking for indexed values
  • realtime filters/subs were not ignoring empty criteria for topics
  • realtime filters/subs were comparing raw against cbor-encoded values. This is now internally consistent, but will change again , see Logs: Indexing/Filtering bugs ref-fvm#1345
  • subscriptions were not matching alternate values of a topic due to byte slice backing array reuse causing them to use the same vale for each alternate

New/revised tests using the event matrix to cover wide variety of filter conditions:

  • TestEthSubscribeLogs
  • TestEthGetFilterLogs - tests EthNewFilter/EthGetFilterChanges combo
  • TestEthGetFilterChanges - tests EthNewFilter/EthGetFilterLogs combo

New test:

  • TestEthNewFilterMergesHistoricWithRealtime

Some noisy reorganisation of event matrix test cases to make them usable to the new tests

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@iand iand requested a review from raulk January 20, 2023 14:14
@iand iand requested a review from a team as a code owner January 20, 2023 14:14
@magik6k
Copy link
Contributor

magik6k commented Jan 20, 2023

Note - #10027 changes how the rpc works / is used, probably worth landing those tests first so that we can have some confidence in the rpc changes later

@iand
Copy link
Contributor Author

iand commented Jan 20, 2023

Note - #10027 changes how the rpc works / is used, probably worth landing those tests first so that we can have some confidence in the rpc changes later

Do you mean landing the tests in this PR or waiting for the tests in 10027?

@iand
Copy link
Contributor Author

iand commented Jan 23, 2023

Added cbor decoding to EthGetTransactionReceipt since we're doing it in the other places we construct an EthLog

Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

Just two comments for you!

}

// Perform all the invocations
messages := invokeAndWaitUntilAllOnChain(t, client, invocations)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it wasn't written in this PR, but invokeAndWaitUntilAllOnChain sends messages into msgChan whether they're reverted or applied. It sets a reverted flag in the msgInTipset object, but this value is never checked. This won't cause any problems if your test setup only uses one miner because there will be no reverts, but this could cause problems with more complicated setups. We should either limit the use of this function to single-miner setups, or we should do something with the reverted flag, so we know a message might not actually be on chain.

Copy link
Contributor Author

@iand iand Jan 25, 2023

Choose a reason for hiding this comment

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

Good point. The ethereum event apis can use this flag, but these single miner tests aren't exercising it. We could add further tests that cause reversions and ensure they are emitted in the ethereum event log apis.

node/impl/full/eth.go Outdated Show resolved Hide resolved
@iand
Copy link
Contributor Author

iand commented Jan 25, 2023

Thanks for the review @geoff-vball. I've removed the debug statement. Are you able to ok this for merging?

@geoff-vball
Copy link
Contributor

Yep, go ahead and merge after the last test passes.

@iand iand enabled auto-merge January 26, 2023 10:19
@iand iand disabled auto-merge January 26, 2023 10:20
@geoff-vball geoff-vball merged commit c3be4f2 into release/v1.20.0 Jan 26, 2023
@geoff-vball geoff-vball deleted the iand/issue-9849-realtime branch January 26, 2023 13:50
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.

3 participants