-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Functions] Mark duplicate request ID log as consumed #9647
Conversation
I see that you haven't updated any README files. Would it make sense to do so? |
err = orm.CreateRequest(id1, ts1, &txHash1) | ||
require.Error(t, err) | ||
// QQQ: the first error type check above passes but his one | ||
// returns a "current transaction is aborted" - why? | ||
require.True(t, errors.Is(err, functions.ErrDuplicateRequestID)) | ||
}) |
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.
Not sure off the top of my head, but one thing you could improve about this is instead of require.True
use:
require.ErorrIs(t, err, functions.ErrDuplicateRequestID)
That will print the actual and expected errors so you can compare, instead of just "Should be true" if it fails.
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.
Oh I see what the problem is... you're looking for 2 sql errors in a row to happen, but that can never happen in tests. The tests run in a special mode where everything is wrapped inside a single transaction. So you can never get more than one error... all queries after the first error will fail with error message "transaction is aborted".
core/services/functions/orm.go
Outdated
@@ -54,7 +60,14 @@ func (o *orm) CreateRequest(requestID RequestID, receivedAt time.Time, requestTx | |||
INSERT INTO ocr2dr_requests (request_id, contract_address, received_at, request_tx_hash, state) | |||
VALUES ($1,$2,$3,$4,$5); | |||
` | |||
return o.q.WithOpts(qopts...).ExecQ(stmt, requestID, o.contractAddress, receivedAt, requestTxHash, IN_PROGRESS) | |||
err := o.q.WithOpts(qopts...).ExecQ(stmt, requestID, o.contractAddress, receivedAt, requestTxHash, IN_PROGRESS) | |||
var pgErr *pgconn.PgError |
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.
This approach seems very fragile to me.
Why not do insert into ... on conflict (request_id) do nothing
, then you check affected rows count returned from Exec(). If the count is zero, then you had a conflict.
@reductionista do you agree?
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.
But then if Exec fails for another reason (e.g. context expired) the row count will be zero as well, right? I want to differentiate a duplicate ID specifically. Another way is to have a transaction with a GET first and then INSERT later.
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.
The err will not be nil, this is how you differentiate.
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.
func (o *orm) CreateRequest(requestID RequestID, receivedAt time.Time, requestTxHash *common.Hash, qopts ...pg.QOpt) error {
stmt := `
INSERT INTO ocr2dr_requests (request_id, contract_address, received_at, request_tx_hash, state)
VALUES ($1,$2,$3,$4,$5) ON CONFLICT (request_id) DO NOTHING;
`
result, err := o.q.WithOpts(qopts...).Exec(stmt, requestID, o.contractAddress, receivedAt, requestTxHash, IN_PROGRESS)
if err != nil {
return err
}
nrows, err := result.RowsAffected()
if err != nil {
return err
}
if nrows == 0 {
return ErrDuplicateRequestID
}
return nil
}
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.
I see, thanks! I'll change to the suggested approach.
c204f79
to
757ebbd
Compare
Do it to avoid a flood of repetitive error when LogBroadcaster re-submits the same log in a loop.
757ebbd
to
f8102ea
Compare
SonarQube Quality Gate |
Do it to avoid a flood of repetitive error when LogBroadcaster re-submits the same log in a loop.
* add an example contract for mercury and log trigger * refactor * refactor * refactor * add a VL contract for log triggered feed lookup upkeep * Change update transmitter balance behaviour (#9624) * Change update transmitter balance behaviour * add test * regen go wrappers and address comments * fix tests * skip coverage for flaky test * remove todo * cleanup merge * regen wrapper * revert ine removal * skip coverage --------- Co-authored-by: FelixFan1992 <fankejin@gmail.com> * Upkeep manager refactor (#9637) * refactor upkeep administrative manager * remove only * fix * fix go build * regen interface * remove only * fix tests * addressed comments * refactor * fix tests * fix error string * fix tests * fix chaincli * Fixes When to Run Live Tests (#9666) * Update AWS action (#9672) * update VL contracts to work with 2.1 (#9673) * update VL contracts to work with 2.1 * fix * Move EVM node pool config (#9656) * Move EVM node pool config * Update after merge * Add test * Update after merge * Cleanup pipeline data references and change back to checkData (#9650) * Cleanup pipeline data references and change back to checkData * fix tests * regen wrappers * regen wrappers * Mercury feeds starts at the first block number not at zero (#9664) * Mercury feeds starts at the first block number not at zero * Update integration test * chainlink-relay => bbcb3a9 * Update core/services/relay/evm/mercury/data_source.go Co-authored-by: Sergei Drugalev <sergei.drugalev@smartcontract.com> * Fix lint * Fix test --------- Co-authored-by: Sergei Drugalev <sergei.drugalev@smartcontract.com> * [Functions] Allowlist support (#9635) * Update mercury telemetry version (#9676) * [Functions] Mark duplicate request ID log as consumed (#9647) Do it to avoid a flood of repetitive error when LogBroadcaster re-submits the same log in a loop. * Refactoring S4 plugin (#9643) * Functions/S4 integration * Fixed whitespace * Removed integration code * Reduce MAX_PERFORM and MAX_CHECK (#9657) * Properly scope logpoller to feedID for mercury (#9680) * [Functions] Use byte array for secrets in RequestData structure (#9677) Co-authored-by: Morgan Kuphal <87319522+KuphJr@users.noreply.github.com> * bump chainlink-env for pod ready improvemeents (#9640) * bump chainlink-env for pod ready improvemeents * bump one more time * function update fix * cleanup checkupkeep and callback (#9651) * cleanup checkupkeep and callback * cleanup * further cleanup * update comment * cleanup * add max revert data size struct * compile * regen wrappers and fix test * fix tests * add test for revert size limit * fix bug * add callback tests * more callback tests * more tests * regen wrappers * fix test * add clarifying comment * fix test * fix chaincli * regen wrappers --------- Co-authored-by: FelixFan1992 <fankejin@gmail.com> * Cleanup TODOs (#9682) * [BCI-1435] Improve EVM Client Readability (#9662) * added type assertion to client * moved chain and client interfaces to common * updated mocks * Added concrete interface of methods in evm client * renamed commontypes to types for headbroadcaster * removed redundent declaration of function in evm client * refactored commontypes to types for txmgr --------- Co-authored-by: Prashant Yadav <34992934+prashantkumar1982@users.noreply.github.com> * Custom configPoller to support multiple Functions plugins (#9648) * custom configPoller to support multiple Functions plugins * removed log * Fixed tests * fixed lint errors * rolled back unneeded lint changes * addressed feedback * fixed error * use imported ConfigPoller interface --------- Co-authored-by: Bolek <1416262+bolekk@users.noreply.github.com> * Use make (#9687) * fix flat fee micro link (#9690) * update url example for mercury (#9691) * Modles.go evm prefix cleanup (#9654) * Modles.go evm prefix cleanup * removed evm prefix for transmitchecker --------- Co-authored-by: Prashant Yadav <34992934+prashantkumar1982@users.noreply.github.com> * [BCF-2299] ocr2 testing (#9684) * Add integration testing OCRV2 helper functions for forwarders * Add forwarders enabled flag to ocr2 smoke test setup func * Add ocr2 with fwds smoke test * Fix forwarders models definitions to match returned json * Fix OCR2TaskJobSpec String method to include ForwardingAllowed field * Change ocr2 fwds smoke test to run for 100 rounds * Improve ocr2_helpers by removing DeployOCRv2Contracts code duplicates * Improve ocr2_helpers by removing CreateOCRv2Jobs code duplicates * Fix fwdrs ocr2 smoke test round val assertion to be of same type * add iLogAutomation geth wrapper (#9665) * Cleanup setConfig on registry (#9692) * Cleanup setConfig * regen wrappers * Allow zero address to be used as proposed admin address (#9695) * Allow zero address to be used as proposed admin address * regen wrappers * fix test * fix reorg upkeep protection to allow bypass by empty blockhash (#9699) * Fix reorg protection to allow bypass via empty blockhash * regen wrappers --------- Co-authored-by: Ryan Hall <RyanRHall@users.noreply.github.com> * Pause checkUpkeep when registry is paused (#9704) * Pause checkUpkeep when registry is paused * regen wrappers * fix test * regen wrappers * cleanup block - conditional trigger naming to be consistent (#9705) * Cleanup block and conditional trigger to be consistent * regen wrappers * regen wrappers * Bump chainlink-starknet (includes latest caigo) (#9137) * bump caigo * Use caigo to encode the point in a canonical way * Update chainlink-starknet * clean up some logging (#9675) * Automation Registrar 2.1 (#9701) * add deployRegistry21 helper * write automation registrar v2.1 * wip * set verify to false for contract verification (#9716) * Remove excess bytes in logging (not necessary to log payload) (#9711) * Functions: connection handler (#9693) * Functions: connection handler prep work * S4 methods * FunctionsConnector tests * Fixed broken test * Addressed PR feedback * Allowlist refactor * Addressed PR feedback * Preparation for Threshold plugin integration with Functions (#9670) * squashing all commits * integrated changes from #9677 * fixed lint error * fixed lint error * fixed proto name conflict * ran protoc * addressed feedback * Addressed feedback * Added decryptionQueueConfig to integration test jobspec * Fix bugs from merge conflict resolution * Initializing S4 plugin (#9725) * Initializing S4 plugin * Addressed PR feedback * Make all error keys in logs consistent (#9718) * clean up evm 20 (#9741) * add a verifiable load contract for log trigger upkeeps * fix encoding issues in registry 2.1 (#9740) * Log event provider (#9583) * log event provider (wip) * fixes 2.1 integration * log event provider + buffer * log event provider: integration test * adding a temp copy from ocr2keepers * increase timeout to stablize test TODO: figure this out w/o timeouts at all * use log struct for check data * move log event provider int test into evm21 * extract 2.1 int tests * aligned default config should fix backfill test instability * fixes and tests * lint * extract logprovider package * extract log packer * stablize test * apply lookback buffer after rate limiting * stablize tests * log struct packing * Registry 2 1 utils contract (#9727) * build AutomationUtils contract and update tests * use report struct as rawReport bytes * update wrappers * add Log struct --------- Co-authored-by: FelixFan1992 <fankejin@gmail.com> * add verifiable load contract for log trigger upkeeps * generate go wrappers * clean up * updates * format * refactor * update * remove build info * format * update * update go script to read results --------- Co-authored-by: Akshay Aggarwal <71980293+infiloop2@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com> Co-authored-by: george-dorin <120329946+george-dorin@users.noreply.github.com> Co-authored-by: Sam <samsondav@protonmail.com> Co-authored-by: Sergei Drugalev <sergei.drugalev@smartcontract.com> Co-authored-by: Bolek <1416262+bolekk@users.noreply.github.com> Co-authored-by: Andrei Smirnov <andrei.smirnov@smartcontract.com> Co-authored-by: kylesmartin <54827727+kylesmartin@users.noreply.github.com> Co-authored-by: Morgan Kuphal <87319522+KuphJr@users.noreply.github.com> Co-authored-by: Tate <tate.exon@smartcontract.com> Co-authored-by: Chia Yong Kang <chiayongkang@hotmail.com> Co-authored-by: Prashant Yadav <34992934+prashantkumar1982@users.noreply.github.com> Co-authored-by: Simson <simsonraj.easvarasakthi@smartcontract.com> Co-authored-by: ilija42 <57732589+ilija42@users.noreply.github.com> Co-authored-by: Ryan Hall <RyanRHall@users.noreply.github.com> Co-authored-by: Blaž Hrastnik <blaz@mxxn.io> Co-authored-by: Lei <lei.shi@smartcontract.com> Co-authored-by: Amir Y <83904651+amirylm@users.noreply.github.com>
Do it to avoid a flood of repetitive error when LogBroadcaster re-submits the same log in a loop.