Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions polkadot/consensus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -770,12 +770,6 @@ impl<C> CreateProposal<C> where C: PolkadotApi {
let result = self.transaction_pool.cull_and_get_pending(BlockId::hash(self.parent_hash), |pending_iterator| {
let mut pending_size = 0;
for pending in pending_iterator {
// skip and cull transactions which are too large.
if pending.encoded_size() > MAX_TRANSACTIONS_SIZE {
unqueue_invalid.push(pending.hash().clone());
continue
}

if pending_size + pending.encoded_size() >= MAX_TRANSACTIONS_SIZE { break }

match block_builder.push_extrinsic(pending.primitive_extrinsic()) {
Expand Down
5 changes: 5 additions & 0 deletions polkadot/transaction-pool/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ error_chain! {
description("Unrecognised address in extrinsic"),
display("Unrecognised address in extrinsic: {}", who),
}
/// Extrinsic too large
TooLarge(got: usize, max: usize) {
description("Extrinsic too large"),
display("Extrinsic is too large ({} > {})", got, max),
}
}
}

Expand Down
13 changes: 11 additions & 2 deletions polkadot/transaction-pool/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ use substrate_runtime_primitives::traits::{Bounded, Checkable, Hash as HashT, Bl
pub use extrinsic_pool::txpool::{Options, Status, LightStatus, VerifiedTransaction as VerifiedTransactionOps};
pub use error::{Error, ErrorKind, Result};

/// 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.

const MAX_TRANSACTION_SIZE: usize = 4 * 1024 * 1024;

/// Type alias for convenience.
pub type CheckedExtrinsic = <UncheckedExtrinsic as Checkable<fn(Address) -> std::result::Result<AccountId, &'static str>>>::Checked;

Expand Down Expand Up @@ -279,14 +284,18 @@ impl<'a, A> txpool::Verifier<UncheckedExtrinsic> for Verifier<'a, A> where
type Error = Error;

fn verify_transaction(&self, uxt: UncheckedExtrinsic) -> Result<Self::VerifiedTransaction> {

if !uxt.is_signed() {
bail!(ErrorKind::IsInherent(uxt))
}

let encoded = uxt.encode();
let (encoded_size, hash) = (encoded.len(), BlakeTwo256::hash(&encoded));
let encoded_size = encoded.len();

if encoded_size > MAX_TRANSACTION_SIZE {
bail!(ErrorKind::TooLarge(encoded_size, MAX_TRANSACTION_SIZE));
}

let hash = BlakeTwo256::hash(&encoded);
debug!(target: "transaction-pool", "Transaction submitted: {}", ::substrate_primitives::hexdisplay::HexDisplay::from(&encoded));

let inner = match uxt.clone().check_with(|a| self.lookup(a)) {
Expand Down
2 changes: 1 addition & 1 deletion substrate/extrinsic-pool/src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ impl<Hash, VEx, S, E> Pool<Hash, VEx, S, E> where
let mut pool = self.pool.write();
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

}
results
}
Expand Down