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

Reject too large transactions - #558

Merged
gavofyork merged 5 commits into
masterfrom
td-txloggingfix
Aug 14, 2018
Merged

Reject too large transactions#558
gavofyork merged 5 commits into
masterfrom
td-txloggingfix

Conversation

@tomusdrw

Copy link
Copy Markdown
Contributor

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

@tomusdrwtomusdrw added A0-please_review Pull request needs code review. M4-core labels Aug 13, 2018
@tomusdrwtomusdrw mentioned this pull request Aug 13, 2018
Comment threadpolkadot/transaction-pool/src/lib.rs Outdated

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

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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));

@pepyakinpepyakinAug 13, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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"

@pepyakinpepyakinAug 13, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 masterAug 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
Closesparitytech#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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tomusdrw@gavofyork@pepyakin