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 1 commit
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
Prev Previous commit
Next Next commit
Use a special kind of extrinsic instead of arbitrary limit.
  • Loading branch information
tomusdrw committed Mar 4, 2020
commit bd14cd294b419e6ebf1c320f143bc77d58b23bab
24 changes: 15 additions & 9 deletions client/basic-authorship/src/basic_authorship.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,13 +412,19 @@ mod tests {
);

futures::executor::block_on(
txpool.submit_at(&BlockId::number(0), {
let mut v = Vec::new();
for i in 0..50 {
v.push(extrinsic(i));
}
v
})
txpool.submit_at(&BlockId::number(0), vec![
extrinsic(0),
extrinsic(1),
Transfer {
amount: Default::default(),
nonce: 2,
from: AccountKeyring::Alice.into(),
to: Default::default(),
}.into_exhaust_tx(),
extrinsic(3),
extrinsic(4),
extrinsic(5),
])
).unwrap();

let mut proposer_factory = ProposerFactory::new(client.clone(), txpool.clone());
Expand All @@ -435,8 +441,8 @@ mod tests {

// then
// block should have some extrinsics although we have some more in the pool.
assert_eq!(block.extrinsics().len(), 16);
assert_eq!(txpool.ready().count(), 50);
assert_eq!(block.extrinsics().len(), 2);
assert_eq!(txpool.ready().count(), 6);
Copy link
Member

Choose a reason for hiding this comment

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

I still would like to see that we build a second block 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.

Then it means that the transaction cannot always exhaust resources. I guess it's fine, I'll change it so that it exhaust resources if it's not the ONLY/FIRST transaction in the block. Then keeping that variant as Transfer extension makes even more sense to me.

}
}

1 change: 0 additions & 1 deletion client/network/src/protocol/sync/blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.

use std::mem;
use std::cmp;
use std::ops::Range;
use std::collections::{HashMap, BTreeMap};
Expand Down
31 changes: 26 additions & 5 deletions test-utils/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,36 @@ impl Transfer {
pub fn into_signed_tx(self) -> Extrinsic {
let signature = sp_keyring::AccountKeyring::from_public(&self.from)
.expect("Creates keyring from public key.").sign(&self.encode()).into();
Extrinsic::Transfer(self, signature)
Extrinsic::Transfer {
transfer: self,
signature,
exhaust_resources: false,
}
}

/// Convert into a signed extrinsic, which will not end up included
/// in block due to resource exhaustion.
#[cfg(feature = "std")]
pub fn into_exhaust_tx(self) -> Extrinsic {
let signature = sp_keyring::AccountKeyring::from_public(&self.from)
.expect("Creates keyring from public key.").sign(&self.encode()).into();
Extrinsic::Transfer {
transfer: self,
signature,
exhaust_resources: true,
}
}
}

/// Extrinsic for test-runtime.
#[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug)]
pub enum Extrinsic {
AuthoritiesChange(Vec<AuthorityId>),
Transfer(Transfer, AccountSignature),
Transfer {
transfer: Transfer,
signature: AccountSignature,
exhaust_resources: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this just a distinct variant in this enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has to share most of the logic with Transfer, since it has to come from some sender and have some nonce. I could simplify it to:

Extrinsic::ExhaustResources(from, nonce)

but I felt it's too arbitrary and it made more sense to just extend transfer.

},
IncludeData(Vec<u8>),
StorageChange(Vec<u8>, Option<Vec<u8>>),
ChangesTrieConfigUpdate(Option<ChangesTrieConfiguration>),
Expand All @@ -130,9 +151,9 @@ impl BlindCheckable for Extrinsic {
fn check(self, _signature: CheckSignature) -> Result<Self, TransactionValidityError> {
match self {
Extrinsic::AuthoritiesChange(new_auth) => Ok(Extrinsic::AuthoritiesChange(new_auth)),
Extrinsic::Transfer(transfer, signature) => {
Extrinsic::Transfer { transfer, signature, exhaust_resources } => {
if sp_runtime::verify_encoded_lazy(&signature, &transfer, &transfer.from) {
Ok(Extrinsic::Transfer(transfer, signature))
Ok(Extrinsic::Transfer { transfer, signature, exhaust_resources })
} else {
Err(InvalidTransaction::BadProof.into())
}
Expand Down Expand Up @@ -165,7 +186,7 @@ impl ExtrinsicT for Extrinsic {
impl Extrinsic {
pub fn transfer(&self) -> &Transfer {
match self {
Extrinsic::Transfer(ref transfer, _) => transfer,
Extrinsic::Transfer { ref transfer, .. } => transfer,
_ => panic!("cannot convert to transfer ref"),
}
}
Expand Down
17 changes: 9 additions & 8 deletions test-utils/runtime/src/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,6 @@ pub fn validate_transaction(utx: Extrinsic) -> TransactionValidity {
/// This doesn't attempt to validate anything regarding the block.
pub fn execute_transaction(utx: Extrinsic) -> ApplyExtrinsicResult {
let extrinsic_index: u32 = storage::unhashed::get(well_known_keys::EXTRINSIC_INDEX).unwrap();
// Arbitrary limit on test-runtime block size.
if extrinsic_index > 15 {
return Err(InvalidTransaction::ExhaustsResources.into());
}
let result = execute_transaction_backend(&utx);
ExtrinsicData::insert(extrinsic_index, utx.encode());
storage::unhashed::put(well_known_keys::EXTRINSIC_INDEX, &(extrinsic_index + 1));
Expand Down Expand Up @@ -241,7 +237,7 @@ pub fn finalize_block() -> Header {
extrinsics_root,
state_root: storage_root,
parent_hash,
digest: digest,
digest,
}
}

Expand All @@ -254,10 +250,15 @@ fn check_signature(utx: &Extrinsic) -> Result<(), TransactionValidityError> {
fn execute_transaction_backend(utx: &Extrinsic) -> ApplyExtrinsicResult {
check_signature(utx)?;
match utx {
Extrinsic::Transfer(ref transfer, _) => execute_transfer_backend(transfer),
Extrinsic::AuthoritiesChange(ref new_auth) => execute_new_authorities_backend(new_auth),
Extrinsic::Transfer { exhaust_resources: true, .. } =>
Err(InvalidTransaction::ExhaustsResources.into()),
Extrinsic::Transfer { ref transfer, .. } =>
execute_transfer_backend(transfer),
Extrinsic::AuthoritiesChange(ref new_auth) =>
execute_new_authorities_backend(new_auth),
Extrinsic::IncludeData(_) => Ok(Ok(())),
Extrinsic::StorageChange(key, value) => execute_storage_change(key, value.as_ref().map(|v| &**v)),
Extrinsic::StorageChange(key, value) =>
execute_storage_change(key, value.as_ref().map(|v| &**v)),
Extrinsic::ChangesTrieConfigUpdate(ref new_config) =>
execute_changes_trie_config_update(new_config.clone()),
}
Expand Down
4 changes: 2 additions & 2 deletions test-utils/runtime/transaction-pool/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ pub fn uxt(who: AccountKeyring, nonce: Index) -> Extrinsic {
nonce,
amount: 1,
};
let signature = transfer.using_encoded(|e| who.sign(e));
Extrinsic::Transfer(transfer, signature.into())
let signature = transfer.using_encoded(|e| who.sign(e)).into();
Extrinsic::Transfer { transfer, signature, exhaust_resources: false }
}