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

Incorrect nonce when node creates a public transaction wrapping a private transaction #10390

Closed
adetante opened this issue Feb 21, 2019 · 3 comments · Fixed by #10391
Closed

Incorrect nonce when node creates a public transaction wrapping a private transaction #10390

adetante opened this issue Feb 21, 2019 · 3 comments · Fixed by #10391
Labels
F2-bug 🐞 The client fails to follow expected behavior. M4-core ⛓ Core client code / Rust.
Milestone
2.5

Comments

@adetante
Copy link

  • Parity Ethereum version: master
  • Operating system: MacOS
  • Installation: built from source
  • Fully synchronized: Not applicable
  • Network: private
  • Restarted: yes

I'm testing Parity Ethereum’s private transactions feature.

I have a private contract successfully deployed. I want to make a state change for this private contract. I create a private transaction, signed by an External Owned Account (which has nonce = 0x1) by sending a signed tx to private_sendTransaction JSON-RPC endpoint.

Once validators signatures collected, the node creates a public transaction wrapping my private transaction. This public transaction is signed by signer_account configurated on this node. This signer_account has nonce = 0x0, but the public transaction crafted by the node has nonce = 0x1 caused by this piece of code:

https://github.com/paritytech/parity-ethereum/blob/master/ethcore/private-tx/src/lib.rs#L365-L371.

This implies that the public transaction is never executed.

I think this is not correct, node should use the current nonce value for signer_account and not the nonce from the original transaction?
@jam10o-new jam10o-new added F2-bug 🐞 The client fails to follow expected behavior. M4-core ⛓ Core client code / Rust. labels Feb 21, 2019
@jam10o-new jam10o-new added this to the 2.4 milestone Feb 21, 2019
@adetante
Copy link
Author

With this fix adetante@87761fd, the correct nonce is included in public transaction and the execution is OK

@jam10o-new
Copy link
Contributor

Feel free to put in a pr for it

@5chdn 5chdn modified the milestones: 2.4, 2.5 Feb 21, 2019
@grbIzl
Copy link
Collaborator

grbIzl commented Feb 22, 2019

@adetante Great catch! We have never tested the scenario, when account signed the transaction is different from the account, which is creating the public transaction. From my point of view, this scenario is absolutely legit. I'll proceed with the review of PR

@adetante adetante mentioned this issue Feb 22, 2019
Merged
soc1c pushed a commit that referenced this issue Feb 22, 2019
@soc1c
fix #10390 (#10391)
fcccbf3
ascjones pushed a commit that referenced this issue Apr 1, 2019
@ascjones
fix #10390 (#10391)
d700988
soc1c pushed a commit that referenced this issue Apr 1, 2019
@niklasad1 @soc1c
2.4.4 more beta backports (#10554)
8f6911a 
* fix(rpc-types): replace uint and hash with `ethereum_types v0.4` (#10217)

* fix(rpc-types): remove uint and hash wrappers

* fix(tests)

* fix(cleanup)

* grumbles(rpc-api): revert `verify_signature`

* revert change of `U64` -> `u64`

* fix(cleanup after bad merge)

* chore(bump ethereum-types)

* fix(bad merge)

* feat(tests ethereum-types): add tests

* chore(update `ethereum-types` to 0.4.2)

* feat(tests for h256)

* chore(rpc): remove `ethbloom` import

Use re-export from `ethereum-types` instead

* fix(bad merge): remove `DefaultAccount` type

* doc(add TODO with issue link)

* chore(bump ethereum-types) (#10396)

Fixes a de-serialization bug in `ethereum-tyes`

* fix(light eth_gasPrice): ask network if not in cache (#10535)

* fix(light eth_gasPrice): ask N/W if not in cache

* fix(bad rebase)

* fix(light account response): update `tx_queue` (#10545)

* fix(bump dependencies) (#10540)

* cargo update -p log:0.4.5

* cargo update -p regex:1.0.5

* cargo update -p parking_lot

* cargo update -p serde_derive

* cargo update -p serde_json

* cargo update -p serde

* cargo update -p lazy_static

* cargo update -p num_cpus

* cargo update -p toml

# Conflicts:
#	Cargo.lock

* tx-pool: check transaction readiness before replacing (#10526)

* Update to vanilla tx pool error

* Prevent a non ready tx replacing a ready tx

* Make tests compile

* Test ready tx not replaced by future tx

* Transaction indirection

* Use StateReadiness to calculate Ready in `should_replace`

* Test existing txs from same sender are used to compute Readiness

* private-tx: Wire up ShouldReplace

* Revert "Use StateReadiness to calculate Ready in `should_replace`"

This reverts commit af9e69c

* Make replace generic so it works with private-tx

* Rename Replace and add missing docs

* ShouldReplace no longer mutable

* tx-pool: update to transaction-pool 2.0 from crates.io

* tx-pool: generic error type alias

* Exit early for first unmatching nonce

* Fix private-tx test, use existing write lock

* Use read lock for pool scoring

* fix #10390 (#10391)

* private-tx: replace error_chain (#10510)

* Update to vanilla tx pool error

* private-tx: remove error-chain, implement Error, derive Display

* private-tx: replace ErrorKind and bail!

* private-tx: add missing From impls and other compiler errors

* private-tx: use original tx-pool error

* Don't be silly cargo
ordian added a commit that referenced this issue Apr 5, 2019
@ordian
Merge branch 'master' into ao-lmdb
135be69 
* master:
  fix #10390 (#10391)
  Fix to_pod storage trie value decoding (#10368)
  revert some changes, could be buggy (#10399)
  version: bump nightly to 2.5 (#10392)
  no-git for publish jobs, empty artifacts dir (#10393)
  fix(jni): bump to jni to 0.11 & remove unsafe impl (#10394)
  chore(bump ethereum-types) (#10396)
  Update to latest mem-db, hash-db and trie-db. (#10314)
  tx pool: always accept local transactions (#10375)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
F2-bug 🐞 The client fails to follow expected behavior. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.5
Development

Successfully merging a pull request may close this issue.

set correct nonce when node creates a public transaction wrapping a private transaction
4 participants
@adetante @grbIzl @5chdn @jam10o-new