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.

Sign up for GitHub

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

Jump to bottom

Use blockhash when determining whether a log event was in an ommer #1382

Merged
merged 8 commits into from
Jun 27, 2019
Merged

Use blockhash when determining whether a log event was in an ommer #1382

merged 8 commits into from
Jun 27, 2019

Conversation

se3000
Copy link
Contributor

@se3000 se3000 commented Jun 19, 2019

No description provided.

@se3000 se3000 force-pushed the bugs/fix-ommer-check branch 4 times, most recently from 9e12d41 to 6429f46 Compare June 25, 2019 05:48
coventry
coventry reviewed Jun 25, 2019
View reviewed changes
core/services/runs.go
@@ -370,6 +370,11 @@ func validateOnMainChain(jr *models.JobRun, taskRun *models.TaskRun, store *stor
return nil
}

func invalidRequest(request models.RunRequest, receipt *models.TxReceipt) bool {
return receipt.Unconfirmed() ||
(request.BlockHash != nil && *request.BlockHash != receipt.BlockHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

How does request.BlockHash = nil happen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Old entries that were in progress as we roll out an update and bring back up the instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like @dimroc said, instead of backfilling we have this check. Could probably get rid of it eventually, but it will smooth rollout now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, guys.

dimroc
dimroc previously requested changes Jun 25, 2019
View reviewed changes
integration/createAndRunJob.test.js Outdated Show resolved Hide resolved
core/store/migrations/migration1560924400/migrate.go Show resolved Hide resolved
core/services/synchronization/stats_pusher.go Outdated Show resolved Hide resolved
@se3000 se3000 changed the title Use blockhash when determining whether a log event was in an ommer WIP: Use blockhash when determining whether a log event was in an ommer Jun 26, 2019
@se3000
do not create a TxReceipt table
4eeacab 
- remove unused gorm tag, as there is no related table
@se3000
isolate the RunRequest in migrations from models
9c03461 
- add explanatory comment about what is happening in the migration
@se3000 se3000 changed the title WIP: Use blockhash when determining whether a log event was in an ommer Use blockhash when determining whether a log event was in an ommer Jun 26, 2019
@se3000 se3000 requested a review from dimroc June 26, 2019 23:59
@coventry
Copy link
Contributor

LGTM. Holding off on merging, since github is saying @dimroc requested changes, although all requests seem resolved, to me.

@se3000 se3000 dismissed dimroc’s stale review June 27, 2019 18:17

Addressed with the last two commits.

@se3000 se3000 merged commit 00fec22 into develop Jun 27, 2019
@se3000 se3000 deleted the bugs/fix-ommer-check branch July 14, 2019 15:05
asoliman92 pushed a commit that referenced this pull request Aug 30, 2024
@dimkouv
Use tests.WaitTimeout for CCIPReader e2e tests (#1382)
a3f61f7 
We've noticed that this tests have failed a few times on the CI.

I've run the tests with `go test ./... -count 100 -failfast` and it
never failed locally.
I assume it runs slow in CI that's why we have some failing cases, I am
replacing `10s` with `tests.WaitTimeout(t)`.
makramkd pushed a commit that referenced this pull request Sep 2, 2024
@dimkouv @makramkd
Use tests.WaitTimeout for CCIPReader e2e tests (#1382)
bbc3c07 
We've noticed that this tests have failed a few times on the CI.

I've run the tests with `go test ./... -count 100 -failfast` and it
never failed locally.
I assume it runs slow in CI that's why we have some failing cases, I am
replacing `10s` with `tests.WaitTimeout(t)`.
github-merge-queue bot pushed a commit that referenced this pull request Sep 2, 2024
@AnieeG @dimkouv
@makramkd @asoliman92
ccip 3076/ccip - port deployment tooling (#14298)
85b33fd 
* porting ccip tooling into chainlink

* make deployment test run

* add missing changes

* fix build error

* make private

* more updates

* changes

* make lint happy

* make lint happy

* Use tests.WaitTimeout for CCIPReader e2e tests (#1382)

We've noticed that this tests have failed a few times on the CI.

I've run the tests with `go test ./... -count 100 -failfast` and it
never failed locally.
I assume it runs slow in CI that's why we have some failing cases, I am
replacing `10s` with `tests.WaitTimeout(t)`.

* core/capabilities/ccip: fix oracle startup logic (#1348)

Ticket: https://smartcontract-it.atlassian.net/browse/CCIP-3078

We can't start bootstrap oracles and plugin oracles on the same
chainlink node. The main reason for this is that we have a singleton
`SingletonPeerWrapper` object which manages all libocr peers for the
application as a whole. Since this singleton only works with a single
peer ID, it creates a single OCR peer, and all streams are created using
this OCR peer. Since streams must have unique config digests for the
same peer object, running a bootstrap oracle and a plugin oracle for the
same config digest will fail because the stream will not be created.

In order to remedy this we match what will be the case in production,
which is:

* Separate bootstrap node, with a peer ID that is either part of the OCR
committee or not (most likely the latter, in order to avoid exporting /
importing the P2P key from one node's DB to another). This bootstrap
node will have to have a DNS name and be publicly accessible over the
internet in order to be accessed by all nodes in the committee, at least
initially, to facilitate peer discovery.
* Plugin node that is more locked down, i.e no public DNS name required.

To enable this, in this PR we:

* Tweak the `launcher` component to launch bootstrap nodes only or
plugin nodes only, and not both. This does NOT rely on the on-chain
bootstrap P2P IDs that are set in the OCR config. This field will be
removed in subsequent PRs.
* Tweak the `OracleCreator` interface to support the above operation

Follow ups:
* Remove the `bootstrapP2PIds` field from the OCR config in
CCIPConfig.sol

* Add changeset

---------

Co-authored-by: dimitris <[email protected]>
Co-authored-by: Makram <[email protected]>
Co-authored-by: asoliman <[email protected]>
Co-authored-by: Abdelrahman Soliman (Boda) <[email protected]>
silaslenihan pushed a commit that referenced this pull request Sep 3, 2024
@AnieeG @dimkouv
@makramkd @asoliman92 @silaslenihan
ccip 3076/ccip - port deployment tooling (#14298)
75aa0d2 
* porting ccip tooling into chainlink

* make deployment test run

* add missing changes

* fix build error

* make private

* more updates

* changes

* make lint happy

* make lint happy

* Use tests.WaitTimeout for CCIPReader e2e tests (#1382)

We've noticed that this tests have failed a few times on the CI.

I've run the tests with `go test ./... -count 100 -failfast` and it
never failed locally.
I assume it runs slow in CI that's why we have some failing cases, I am
replacing `10s` with `tests.WaitTimeout(t)`.

* core/capabilities/ccip: fix oracle startup logic (#1348)

Ticket: https://smartcontract-it.atlassian.net/browse/CCIP-3078

We can't start bootstrap oracles and plugin oracles on the same
chainlink node. The main reason for this is that we have a singleton
`SingletonPeerWrapper` object which manages all libocr peers for the
application as a whole. Since this singleton only works with a single
peer ID, it creates a single OCR peer, and all streams are created using
this OCR peer. Since streams must have unique config digests for the
same peer object, running a bootstrap oracle and a plugin oracle for the
same config digest will fail because the stream will not be created.

In order to remedy this we match what will be the case in production,
which is:

* Separate bootstrap node, with a peer ID that is either part of the OCR
committee or not (most likely the latter, in order to avoid exporting /
importing the P2P key from one node's DB to another). This bootstrap
node will have to have a DNS name and be publicly accessible over the
internet in order to be accessed by all nodes in the committee, at least
initially, to facilitate peer discovery.
* Plugin node that is more locked down, i.e no public DNS name required.

To enable this, in this PR we:

* Tweak the `launcher` component to launch bootstrap nodes only or
plugin nodes only, and not both. This does NOT rely on the on-chain
bootstrap P2P IDs that are set in the OCR config. This field will be
removed in subsequent PRs.
* Tweak the `OracleCreator` interface to support the above operation

Follow ups:
* Remove the `bootstrapP2PIds` field from the OCR config in
CCIPConfig.sol

* Add changeset

---------

Co-authored-by: dimitris <[email protected]>
Co-authored-by: Makram <[email protected]>
Co-authored-by: asoliman <[email protected]>
Co-authored-by: Abdelrahman Soliman (Boda) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coventry coventry coventry left review comments

@dimroc dimroc dimroc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@se3000 @coventry @dimroc