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

remove null signatures#11335

Merged
tomusdrw merged 9 commits into
masterfrom
ao-remove-support-for-null-signatures
Dec 13, 2019
Merged

remove null signatures#11335
tomusdrw merged 9 commits into
masterfrom
ao-remove-support-for-null-signatures

Conversation

@ordian
Copy link
Copy Markdown
Member

@ordian ordian commented Dec 13, 2019

eip86Transition flag was removed in #9140, this PR removes the null-signature as a follow-up cleanup.

@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. labels Dec 13, 2019
@ordian ordian added this to the 2.7 milestone Dec 13, 2019
ordian
ordian commented Dec 13, 2019
View reviewed changes
Comment thread ethcore/types/src/transaction/transaction.rs Outdated
Comment thread json/src/transaction.rs Outdated
Comment thread json/src/spec/builtin.rs
@ordian ordian changed the title remove support for null signatures remove null signatures Dec 13, 2019
dvdplm
dvdplm reviewed Dec 13, 2019
View reviewed changes
Comment thread ethcore/types/src/transaction/transaction.rs Outdated
Comment thread ethcore/types/src/transaction/transaction.rs
pub fn verify_basic(&self, check_low_s: bool, chain_id: Option<u64>) -> Result<(), error::Error> {
// Disallow unsigned transactions.
if self.is_unsigned() {
return Err(parity_crypto::publickey::Error::InvalidSignature.into());
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm Dec 13, 2019

Choose a reason for hiding this comment

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

Should we have a Error::Unsigned you think?

Comment thread ethcore/types/src/transaction/transaction.rs
Comment thread json/src/transaction.rs Outdated
Comment thread ethcore/types/src/transaction/transaction.rs
tomusdrw
tomusdrw approved these changes Dec 13, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

Comment thread ethcore/types/src/transaction/transaction.rs
/// Fake address for unsigned transactions as defined by EIP-86.
/// Fake address for unsigned transactions as defined by Legacy EIP-86.
#[cfg(any(test, feature = "test-helpers"))]
pub const UNSIGNED_SENDER: Address = H160([0xff; 20]);
Copy link
Copy Markdown
Collaborator

@tomusdrw tomusdrw Dec 13, 2019

Choose a reason for hiding this comment

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

Why not remove this entirely?

Copy link
Copy Markdown
Member Author

@ordian ordian Dec 13, 2019

Choose a reason for hiding this comment

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

What to do with

  1. https://github.com/paritytech/parity-ethereum/blob/6e76be7fad1cf1ae40773183084f13c37acabdde/ethcore/machine/src/machine.rs#L155-L156
  2. https://github.com/paritytech/parity-ethereum/blob/6e76be7fad1cf1ae40773183084f13c37acabdde/ethcore/machine/src/externalities.rs#L262
  3. (probably just remove) https://github.com/paritytech/parity-ethereum/blob/6e76be7fad1cf1ae40773183084f13c37acabdde/ethcore/types/src/transaction/transaction.rs#L503-L505

Copy link
Copy Markdown
Collaborator

@tomusdrw tomusdrw Dec 13, 2019

Choose a reason for hiding this comment

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

  1. Try to remove Option<> from contract_address - I suppose it will always be set to Some.
  2. Remove along with keep_unsigned_nonce parameter from schedule, it's always set to false anyway.
  3. Just remove along with fn is_unsigned methods from all *Transaction structs.

Copy link
Copy Markdown
Collaborator

@tomusdrw tomusdrw Dec 13, 2019

Choose a reason for hiding this comment

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

Just checked and it seems 1. needs to stay, as it's being used by block-reward calls inside the engine, so let's leave it as is. So I guess we can keep the constant, but try to remove everything else.

@ordian
Copy link
Copy Markdown
Member Author

ordian commented Dec 13, 2019

I don't have time to investigate this failure https://gitlab.parity.io/parity/parity-ethereum/-/jobs/331339. My suggestion is to merge it as is, more cleanup can be done in a followup PR.

@dvdplm
Copy link
Copy Markdown
Collaborator

dvdplm commented Dec 13, 2019

My suggestion is to merge it as is, more cleanup can be done in a followup PR.

Works for me. @tomusdrw?

@tomusdrw
Copy link
Copy Markdown
Collaborator

tomusdrw commented Dec 13, 2019

Yeah, sounds good! Let's just not forget that :)

@tomusdrw tomusdrw merged commit 6353586 into master Dec 13, 2019
@tomusdrw tomusdrw deleted the ao-remove-support-for-null-signatures branch December 13, 2019 17:12
ordian added a commit that referenced this pull request Dec 13, 2019
@ordian
Merge branch 'master' into ao-verify-basic-local-txns
87e271f 
* master:
  remove null signatures (#11335)
dvdplm pushed a commit that referenced this pull request Dec 13, 2019
@ordian @dvdplm
remove null signatures (#11335)
106b5b9 
* tx: clean up legacy eip-86 based null signature

* tx: add a test for null signature rejection

* tx: revert json txn changes

* fix evmbin bench build

* tx: put UNSIGNED_SENDER behind 'test-helpers' feature

* Revert "tx: put UNSIGNED_SENDER behind 'test-helpers' feature"

This reverts commit 1dde478.

* tx: add comment for null_sign

* even more cleanup

* Revert "even more cleanup"

This reverts commit be29f03.
dvdplm pushed a commit that referenced this pull request Dec 13, 2019
@ordian @dvdplm
remove null signatures (#11335)
30c0ef2 
* tx: clean up legacy eip-86 based null signature

* tx: add a test for null signature rejection

* tx: revert json txn changes

* fix evmbin bench build

* tx: put UNSIGNED_SENDER behind 'test-helpers' feature

* Revert "tx: put UNSIGNED_SENDER behind 'test-helpers' feature"

This reverts commit 1dde478.

* tx: add comment for null_sign

* even more cleanup

* Revert "even more cleanup"

This reverts commit be29f03.
This was referenced Dec 14, 2019
Merged
Merged
@dvdplm dvdplm mentioned this pull request Dec 15, 2019
Closed
@dvdplm
Copy link
Copy Markdown
Collaborator

dvdplm commented Dec 15, 2019

Yeah, sounds good! Let's just not forget that :)

#11339

/cc @ordian

dvdplm added a commit that referenced this pull request Dec 15, 2019
@dvdplm
Remove SecretStore migration code and invite users to use 2.6 to migr…
c674b09 
…ate their data instead

Merge branch 'master' into dp/chore/kvdb-no-default-column

* master:
  tx-q: enable basic verification of local transactions (#11332)
  remove null signatures (#11335)
  ethcore/res: activate agharta on classic 9573000 (#11331)
  [secretstore] migrate to version 4 (#11322)
  Enable EIP-2384 for ice age hard fork (#11281)
  Fix atomicity violation in network-devp2p (#11277)
s3krit pushed a commit that referenced this pull request Dec 16, 2019
@dvdplm @s3krit
V2.5.12 stable (#11336)
29ebddc 
* Enable EIP-2384 for ice age hard fork (#11281)
* ethcore/res: activate agharta on classic 9573000 (#11331)
* Istanbul HF in xDai (2019-12-12) (#11299)
* Istanbul HF in POA Core (2019-12-19) (#11298)
* Istanbul HF in POA Sokol (2019-12-05) (#11282)
* Activate ecip-1061 on kotti and mordor (#11338)
* Enable basic verification of local transactions (#11332)
* Disallow EIP-86 style null signatures for transactions outside tests (#11335)
s3krit pushed a commit that referenced this pull request Dec 16, 2019
@dvdplm @s3krit
V2.6.7 beta (#11337)
ba1b879 
* Enable EIP-2384 for ice age hard fork (#11281)
* ethcore/res: activate agharta on classic 9573000 (#11331)
* Istanbul HF in xDai (2019-12-12) (#11299)
* Istanbul HF in POA Core (2019-12-19) (#11298)
* Istanbul HF in POA Sokol (2019-12-05) (#11282)
* Activate ecip-1061 on kotti and mordor (#11338)
* Enable basic verification of local transactions (#11332)
* Disallow EIP-86 style null signatures for transactions outside tests (#11335)
* SecretStore database migration to v4 (#11322)
@ackintosh ackintosh mentioned this pull request Jan 16, 2020
Merged
@ordian ordian mentioned this pull request Feb 14, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@tomusdrw tomusdrw tomusdrw approved these changes

@niklasad1 niklasad1 niklasad1 left review comments

@dvdplm dvdplm dvdplm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API.

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

4 participants

@ordian @dvdplm @tomusdrw @niklasad1