Add transaction pool to Aura and Babe import queue #3225

marcio-diaz · 2019-07-27T14:04:12Z

We need to submit report transactions when we discover an equivocation while importing (see here for definition of equivocation).

As a starting point, this PR adds the transaction pool to Aura and Babe import queue (and check_header).

tomaka · 2019-07-27T16:12:02Z

Please add a comment in the Babe code explaining why we're passing the transaction pool. The comment can later be removed when we actually use it, but for now I'd prefer if there was an explanation.

We merge many PRs that add code "for later", and we end up not using that code, and then nobody remembers why it's there nor dares to remove it.

bkchr · 2019-07-27T16:23:39Z

I would go even further as @tomaka and say, why not build directly the full functionality? This is no big change, which can be reviewed together with the actual functionality.

marcio-diaz · 2019-07-27T20:28:51Z

I would go even further as @tomaka and say, why not build directly the full functionality? This is no big change, which can be reviewed together with the actual functionality.

The full functionality may be too much for a PR. But sure, I can push more code to give a bigger picture to the reviewers.

marcio-diaz · 2019-07-29T14:38:22Z

I added some more code, but looks like I can't go much further. The full functionality depends on #3063 and #3137 at least.

Demi-Marie

The completed code looks good so far, but there is a long ways to go.

core/consensus/babe/primitives/src/equivocation.rs

core/finality-grandpa/Cargo.toml

andresilva

This is not reviewable. You bundled a bunch of changes from other PRs into a single commit (599c8b1). If you depend on other PRs the correct way to do it is to create a branch that descends from the one you depend on. FWIW I'm fine with merging just the changes to introduce the transaction pool to Babe (and Aura!) without depending on any pending PRs, provided that we comment it like @tomaka suggested.

marcio-diaz · 2019-07-29T16:09:55Z

This is not reviewable. You bundled a bunch of changes from other PRs into a single commit (599c8b1). If you depend on other PRs the correct way to do it is to create a branch that descends from the one you depend on. FWIW I'm fine with merging just the changes to introduce the transaction pool to Babe (and Aura!) without depending on any pending PRs, provided that we comment it like @tomaka suggested.

I have not intention to merge the other PR, just to use it as a guide.

I'll follow the suggestions of @tomaka and @andresilva and just introduce the changes for the tx pool in Aura and Babe as was originally intended.

old

marcio-diaz · 2019-07-31T10:27:32Z

This is ready for review, I added the tx pool to Aura too.

andresilva

When you actually want to use the transaction pool you will need to add some trait bounds to T. Right now it is just a generic type from which we cannot infer anything. I think we should add the correct bounds in this PR so that it is usable (even though it won't be used in this PR).

core/consensus/babe/src/lib.rs

Demi-Marie

LGTM

andresilva

As discussed offline this is not usable as is, since the T type is not bound by anything. @marcio-diaz did some changes to fix this on another branch not sure if we wants to get them here or not.

marcio-diaz · 2019-08-09T09:45:22Z

As discussed offline this is not usable as is, since the T type is not bound by anything. @marcio-diaz did some changes to fix this on another branch not sure if we wants to get them here or not.

Now, that I read the comment again, it feels like an arbitrary decision.
T is useless with or without bound, and adding it doesn't look perfect either.
IMO, the point of splitting the other big PR was to facilitate review and merging.
If anyways we are going to merge this in a few days, then why not merging it now?

andresilva · 2019-08-09T14:08:42Z

IMO my comment is justified, this PR is titled "Add transaction pool", and it adds a type T which does not relate at all to a TransactionPool, so as a logical unit of work it doesn't accomplish what it says it does. You added a trait SubmitExtrinsic here (#3314) which solves this exact problem, my suggestion was to add something like that here in this PR. If that is done then in this PR we have a small set of changes and something that is logically correct (even though it is unused). I am OK with merging this anyway and just doing those changes in the other PR to avoid more work on this, but it has conflicts now.

marcio-diaz · 2019-08-12T08:05:57Z

@andresilva Thanks, conflicts solved.

solved

gavofyork · 2019-08-12T10:52:39Z

@andresilva if you're happy with it, then please merge.

marcio-diaz added the A0-please_review Pull request needs code review. label Jul 27, 2019

marcio-diaz requested review from Demi-Marie, bkchr and tomaka July 27, 2019 14:38

marcio-diaz mentioned this pull request Jul 29, 2019

Equivocation misbehaviour reports #2923

Closed

marcio-diaz force-pushed the marcio/add-tx-pool-to-import-queue branch from 49d94c7 to a224ac5 Compare July 29, 2019 14:29

Demi-Marie previously requested changes Jul 29, 2019

View reviewed changes

core/consensus/babe/primitives/src/equivocation.rs Outdated Show resolved Hide resolved

core/finality-grandpa/Cargo.toml Outdated Show resolved Hide resolved

core/finality-grandpa/Cargo.toml Outdated Show resolved Hide resolved

andresilva added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jul 29, 2019

andresilva previously requested changes Jul 29, 2019

View reviewed changes

marcio-diaz force-pushed the marcio/add-tx-pool-to-import-queue branch from a224ac5 to 4b8384c Compare July 29, 2019 16:22

marcio-diaz changed the title ~~Add transaction pool to Babe import queue~~ Add transaction pool to Aura and Babe import queue Jul 30, 2019

marcio-diaz added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 30, 2019

marcio-diaz requested a review from andresilva July 30, 2019 10:49

marcio-diaz force-pushed the marcio/add-tx-pool-to-import-queue branch from 1c15e2c to 5c7c389 Compare July 30, 2019 11:45

marcio-diaz requested a review from Demi-Marie July 31, 2019 10:28

andresilva reviewed Jul 31, 2019

View reviewed changes

core/consensus/babe/src/lib.rs Show resolved Hide resolved

Demi-Marie approved these changes Jul 31, 2019

View reviewed changes

Demi-Marie requested a review from andresilva August 1, 2019 16:36

Demi-Marie approved these changes Aug 1, 2019

View reviewed changes

gavofyork removed the A0-please_review Pull request needs code review. label Aug 7, 2019

gavofyork added the A7-looksgoodcantmerge label Aug 7, 2019

andresilva previously requested changes Aug 8, 2019

View reviewed changes

andresilva added A3-needsresolving and removed A7-looksgoodcantmerge labels Aug 8, 2019

seerscode added 7 commits August 12, 2019 09:55

Add transaction pool to babe import queue

b8eb38a

Add transaction pool to Babe check header

bbc9d19

Fix tests

8be77ef

Add tx pool to Aura import_queue

cf77c26

Fix tests, node-template

5e29bdf

Add comments regarding unused _transaction_pool

3fbdd55

Make tx pool optional in check_header

c872ede

marcio-diaz force-pushed the marcio/add-tx-pool-to-import-queue branch from 25c6d8c to c872ede Compare August 12, 2019 08:04

marcio-diaz added A8-looksgood and removed A3-needsresolving labels Aug 12, 2019

andresilva merged commit 6fbca29 into master Aug 13, 2019

andresilva deleted the marcio/add-tx-pool-to-import-queue branch August 13, 2019 09:44

tomaka mentioned this pull request Aug 16, 2019

Propagate substrate#3225 paritytech/polkadot#376

Closed

Add transaction pool to Aura and Babe import queue #3225

Add transaction pool to Aura and Babe import queue #3225

Uh oh!

Conversation

marcio-diaz commented Jul 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomaka commented Jul 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented Jul 27, 2019

Uh oh!

marcio-diaz commented Jul 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcio-diaz commented Jul 29, 2019

Uh oh!

Demi-Marie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andresilva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcio-diaz commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcio-diaz commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Demi-Marie left a comment

Choose a reason for hiding this comment

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

marcio-diaz commented Aug 9, 2019

Uh oh!

andresilva commented Aug 9, 2019

Uh oh!

marcio-diaz commented Aug 12, 2019

Uh oh!

gavofyork commented Aug 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

marcio-diaz commented Jul 27, 2019 •

edited

Loading

tomaka commented Jul 27, 2019 •

edited

Loading

marcio-diaz commented Jul 27, 2019 •

edited

Loading

andresilva left a comment •

edited

Loading

marcio-diaz commented Jul 29, 2019 •

edited

Loading

marcio-diaz commented Jul 31, 2019 •

edited

Loading