-
Notifications
You must be signed in to change notification settings - Fork 5
NONEVM-3317: LogPoller misc patches #515
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
base: main
Are you sure you want to change the base?
Conversation
| txHash string | ||
| txLT uint64 | ||
| msgIndex int64 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: aligning with db dedub key(chain id is already unique per store)
| :master_block_seqno, | ||
| NOW() | ||
| ) ON CONFLICT (tx_hash, tx_lt, msg_index) DO NOTHING | ||
| ) ON CONFLICT DO NOTHING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: removing to use unique index in the table automatically
| errsOut := make(chan error) | ||
|
|
||
| var wg sync.WaitGroup | ||
| var txCount, logCount atomic.Int32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: unused, metrics points inserted separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds deduplication logic for log entries across multiple filters and enhances metrics tracking in the TON LogPoller. The changes address the issue where multiple filters monitoring the same blockchain event can create duplicate log entries in storage, ensuring only unique events are returned to consumers while allowing storage of filter-specific copies.
Changes:
- Modified database unique constraint from
(tx_hash, tx_lt, msg_index)to(chain_id, filter_id, tx_hash, tx_lt, msg_index)to allow multiple filters to store the same event - Added query-time deduplication in both PostgreSQL and in-memory stores to return unique events
- Enhanced parser metrics to track transactions, messages, and matched logs with detailed labels
- Updated documentation and comments explaining filter timing and performance considerations
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/logpoller/store/postgres/queryparser.go | Added DISTINCT to SELECT queries and explanatory comments about deduplication |
| pkg/logpoller/store/postgres/queryparser_test.go | Updated test expectations to include DISTINCT in SQL queries |
| pkg/logpoller/store/postgres/logs.go | Modified ON CONFLICT clause and added in-memory deduplication logic after query |
| pkg/logpoller/store/memory/logs.go | Updated deduplication key to include filter_id and added query-time deduplication |
| pkg/logpoller/store/memory/logs_test.go | Added comprehensive tests for deduplication behavior |
| pkg/logpoller/parser.go | Added context parameter to parse functions, integrated new metrics, and updated comments |
| pkg/logpoller/parser_test.go | Updated test calls to include context parameter |
| pkg/logpoller/metrics.go | Added parser pipeline metrics for transactions, messages, and matched logs |
| pkg/logpoller/interfaces.go | Added documentation about filter timing behavior |
| pkg/logpoller/filter.go | Enhanced comments explaining filter registration timing |
| pkg/logpoller/config.go | Added performance documentation about PollPeriod and PageSize |
| pkg/logpoller/block.go | Clarified comment about genesis block handling |
| pkg/ccip/chainaccessor/event.go | Updated TODO comment with date |
| integration-tests/logpoller/testdata/create_ton_logpoller_tables.sql | Modified unique index to include chain_id and filter_id |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
integration-tests/logpoller/testdata/create_ton_logpoller_tables.sql
Outdated
Show resolved
Hide resolved
pkg/logpoller/config.go
Outdated
| // NOTE: when adding new fields, please update ApplyDefaults, DefaultConfigSet, and ValidateConfig accordingly. | ||
| // Also check toml_test.go TestNewDecodedTOMLConfig() to ensure new fields are tested there. | ||
| // | ||
| // Performance Note: Tick processing time must complete within PollPeriod to avoid falling behind chain head. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we ever warning log or block another tick from starting if there is currently a tick already in progress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there can be cumulative delay if a loop takes longer than block time. Added a warning log for this case to improve operational visibility. Also updated the config comment to clarify that concurrent ticks cannot occur
The service loop is already blocking, GoTick from chainlink-common executes ticks synchronously:
https://github.com/smartcontractkit/chainlink-common/blob/3c6cb30b141b00d67aa3f5b65d798d89ce79f44b/pkg/services/service.go#L96-L108
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_filters_name ON ton.log_poller_filters (chain_id, name) WHERE NOT is_deleted; | ||
| CREATE INDEX IF NOT EXISTS idx_filters_address_msgtype ON ton.log_poller_filters(address, msg_type); | ||
| CREATE INDEX IF NOT EXISTS idx_filters_address_msgtype ON ton.log_poller_filters(chain_id, address, msg_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: adding missing fields in the index, core migration update are being tracked in here: https://github.com/smartcontractkit/chainlink/compare/jade/ton-lp-table-fixes?expand=1
Currently there are no breaking changes on the fields, so planning to merge it before the release including features such as log pruning.
Uh oh!
There was an error while loading. Please reload this page.