Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@drahnr
Copy link
Contributor

@drahnr drahnr commented Mar 10, 2020

Changes in order to allow writing data during block import to the offchain database, so that the offchain worker has access to this data.

The implementation introduces a new interface accessible from runtime.

Related #3722

polkadot companion: paritytech/polkadot#1025

@drahnr drahnr self-assigned this Mar 10, 2020
@drahnr drahnr force-pushed the bernhard/deterministic-bookkeeping-issue-3722 branch from ed6990d to 5d11ae9 Compare March 10, 2020 07:54
@gavofyork gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 10, 2020
@drahnr drahnr requested a review from tomusdrw March 10, 2020 11:00
@drahnr drahnr force-pushed the bernhard/deterministic-bookkeeping-issue-3722 branch from 075c9f9 to d5ff49b Compare March 11, 2020 10:17
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good overall, I'd change the logic a bit though.

  1. import_notification_stream is left as it works now - not guaranteed for every block, and also not running for initial sync. Has some extra data in the notification though.
    2 all_blocks_import_notification_stream - a more "lightweight" stream, but guaranteed to run for each and every imported block. This is what offchain workers should be using now. Additionally the information if it was an initial sync or not should be passed as parameter in the stream, so that we can for instance keep backward compatibility for extisting offchain workers (the filtering would be done within the runtime though, not outside).

@drahnr drahnr force-pushed the bernhard/deterministic-bookkeeping-issue-3722 branch from f4868db to e9ca0eb Compare March 11, 2020 18:23
@drahnr drahnr force-pushed the bernhard/deterministic-bookkeeping-issue-3722 branch from f059032 to 28a464c Compare March 16, 2020 13:06
@drahnr drahnr force-pushed the bernhard/deterministic-bookkeeping-issue-3722 branch 2 times, most recently from c0dfe8b to 1b76596 Compare March 24, 2020 08:32
@drahnr drahnr requested a review from tomusdrw March 24, 2020 09:05
@drahnr drahnr force-pushed the bernhard/deterministic-bookkeeping-issue-3722 branch from 74d1137 to f09046b Compare March 30, 2020 13:51
@drahnr drahnr linked an issue Mar 30, 2020 that may be closed by this pull request
@drahnr drahnr marked this pull request as ready for review March 30, 2020 14:08
@drahnr drahnr requested review from NikVolf and pepyakin as code owners March 30, 2020 14:08
@drahnr
Copy link
Contributor Author

drahnr commented Mar 30, 2020

Note that this contains changes of #5455

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

None were critical to request changes, but I had some comments that preferably need addressing before merge.

@drahnr drahnr changed the title feat/ocw/bookkeeping - ref #3722 feat/ocw/bookkeeping Apr 23, 2020
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

A bunch of docs improvements, functionally it looks good.

}

impl Externalities for BasicExternalities {
fn set_offchain_storage(&mut self, _key: &[u8], _value: Option<&[u8]>) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it's better to work with borrowed data, cause we can avoid a clone if indexing is disabled. That said we should probably unify the API (and introduce Storage*Ref to work with slices). I'd leave it as a separate PR though.

@rphmeier
Copy link
Contributor

Companion and this look good, I think we can merge once CI completes here. cc @TriplEight companion PR task may not be working correctly; I've tried restarting it but we'll see if it works.

@TriplEight
Copy link
Contributor

@rphmeier I've checked and it seems to be caused by GitHub availability issues.

@gnunicorn gnunicorn merged commit f267590 into master Apr 24, 2020
@gnunicorn gnunicorn deleted the bernhard/deterministic-bookkeeping-issue-3722 branch April 24, 2020 14:46
@cheme
Copy link
Contributor

cheme commented May 4, 2020

Quick question, should the overlay maintain a commit and prospective state?
(currently rejected extrinsic will index, but that may not be an issue (if reorg is not this is maybe not either).

@rphmeier
Copy link
Contributor

rphmeier commented May 4, 2020

Good point - although at the moment our use-cases are in on_initialize/on_finalize only, we should be able to handle rollback better.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Offchain Workers: deterministic bookkeeping