-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help a reviewer out and point me to where the type safety happens please?
@@ -439,9 +430,6 @@ impl From<SignedTransaction> for UnverifiedTransaction { | |||
impl SignedTransaction { | |||
/// Try to verify transaction and recover sender. | |||
pub fn new(transaction: UnverifiedTransaction) -> Result<Self, parity_crypto::publickey::Error> { | |||
if transaction.is_unsigned() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the aforementioned type safety is here, SignedTransaction::new
will not accept unsigned transactions after #11335
match basic_test(&create_test_block_with_data(&bad_header, &eip86_transactions, &good_uncles), engine) { | ||
Err(Error::Transaction(ref e)) if e == &parity_crypto::publickey::Error::InvalidSignature.into() => (), | ||
e => panic!("Block verification failed.\nExpected: Transaction Error (Invalid Signature)\nGot: {:?}", e), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was required (as some other removed code) to account for eip86 which we no longer support
return Ok(ContractCreateResult::Failed) | ||
} | ||
if let Err(e) = self.state.inc_nonce(&self.origin_info.address) { | ||
warn!(target: "ext", "Database corruption encountered: {:?}", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to a warning
@@ -419,25 +419,6 @@ mod tests { | |||
} | |||
} | |||
|
|||
#[test] | |||
fn should_disallow_unsigned_transactions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep the test. I guess the failure is going to be at a dfiferent level now, but still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or did you mean something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I just think it's worth to keep the test in it's current form, as it's covering a bit more than constructor of SignedTransaction
. I think it was added based on an actual previous bug, so keeping it as a way to prevent regressions should hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was added in #8802 and it doesn't make sense anymore. I removed it because it actually fails on this branch, since it creates a null-signed UnverifiedTransaction
and calls verify_transaction_basic
expecting it to reject it.
But this PR removes this rejection logic, since it's not needed as we don't allow SignedTransaction
s with null-signatures.
Now if I modify the test to convert a transaction to a SignedTransaction
, then it's not different than the test linked above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion happens within engine/machine
, doesn't it (via verify_transaction
). IMHO still worth to check the machine/engine integration with this, but not super strong on this. If you disagree - then fine, let's remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion happens in the verification queue when we convert Unverified
block to PreverifiedBlock
https://github.com/paritytech/parity-ethereum/blob/856a0755888a30d4dc52a68cb2638a8bfd5786ac/ethcore/verification/src/verification.rs#L100
Added dd4bbf3, let me know what you think.
* master: (27 commits) Faster kill_garbage (#11514) [EngineSigner]: don't sign message with only zeroes (#11524) fix compilation warnings (#11522) [ethcore cleanup]: various unrelated fixes from `#11493` (#11507) Add benchmark for transaction execution (#11509) Add Smart Contract License v1.0 Misc fixes (#11510) [dependencies]: unify `rustc-hex` (#11506) Activate on-chain randomness in POA Sokol (#11505) Grab bag of cleanup (#11504) Implement eth/64 (EIP-2364) and drop support for eth/62 (#11472) [dependencies]: remove `util/macros` (#11501) OpenEthereum bootnodes are added (#11499) [ci benches]: use `all-features` (#11496) [verification]: make test-build compile standalone (#11495) complete null-signatures removal (#11491) Include the seal when populating the header for a new block (#11475) fix compilation warnings (#11492) cargo update -p cmake (#11490) update to published rlp-derive (#11489) ...
Closes #11339.