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

Conversation

@tomusdrw
Copy link
Contributor

Move rejection from consensus to pool verifier, so that large transactions are not propagated around.

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. M4-core labels Aug 13, 2018
@tomusdrw tomusdrw mentioned this pull request Aug 13, 2018

/// Maximal size of a single encoded extrinsic.
///
/// See also substrate-consensus::MAX_TRANSACTIONS_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm was unable to find this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I meant polkadot-consensus.

let mut results = Vec::with_capacity(hashes.len());
for hash in hashes {
results.push(pool.remove(hash, is_valid));
results.push(pool.remove(hash, !is_valid));
Copy link
Contributor

@pepyakin pepyakin Aug 13, 2018

Choose a reason for hiding this comment

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

I'm not sure I follow this one. Why is it inverted here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The transaction-pool API expects an inverted bool (is_invalid) - that was a bug that was causing the transactions to be logged as "canceled" instead of "invalid"

Copy link
Contributor

@pepyakin pepyakin Aug 13, 2018

Choose a reason for hiding this comment

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

Oh, maybe we should rename it then, since it's presently named is_valid? This is actually where my confusion comes from : )

https://github.com/paritytech/polkadot/blob/e89b8aa7e4ceb2b8f1348e027d61ebb9c820eec4/polkadot/transaction-pool/src/lib.rs#L389

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the composition looks like this:

polkadot-transaction-pool -> substrate-extrinsic-pool -> [parity-]transaction-pool

The code in polkadot transaction pool is fine it uses is_valid and passes it further to extrinsics pool. Extrinsics pool uses the external transaction-pool (take from ethereum/parity) and that one is using is_invalid (https://docs.rs/transaction-pool/1.12.1/transaction_pool/struct.Pool.html#method.remove) hence the inversion here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see it now! I mistakingly assumed that transaction_pool as txpool refers to polkadot-transaction-pool

@gavofyork gavofyork merged commit 1b325b2 into master Aug 14, 2018
@gavofyork gavofyork deleted the td-txloggingfix branch August 14, 2018 11:13
dvdplm added a commit that referenced this pull request Aug 14, 2018
* master:
  Update some outdated slashing tests in runtime (#565)
  Update libp2p (#559)
  Reject too large transactions (#558)
  cli: add min-peers and max-peers (#557)
  RPC: query historical storage entries (#537)
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
* Set version as V0.9.10

* Change telemetery chain name

* fix rpc trading_pairs overflow

* fix rpc quotations overflow

* Use account instead of authority_id in xsession initializetion

Closes paritytech#558

* Update substrate

* Update genesis

* Add tty password

* Modified team & council public key

* Update concil as 5

* Update genesis btc-header

* Update team and council account

* update substrate

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

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants