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

From/To in transaction traces seem to be wrong for delegatecall #7166

Closed
karalabe opened this issue Nov 29, 2017 · 8 comments · Fixed by #7568
Closed

From/To in transaction traces seem to be wrong for delegatecall #7166

karalabe opened this issue Nov 29, 2017 · 8 comments · Fixed by #7568
Labels
F2-bug 🐞 The client fails to follow expected behavior. M6-rpcapi 📣 RPC API. P5-sometimesoon 🌲 Issue is worth doing soon.
Milestone

Comments

@karalabe
Copy link

I'm currently reworking the transaction tracing in go-ethereum, and I've seen some discrepancies between our results and the Parity results from EtherScan. After a quick look, it seems to me that Parity's traces are wrong, but please correct me if I'm wrong.

A sample transaction is https://ropsten.etherscan.io/tx/0x427dfdbe8077d2901a51767672db3d4c37b7b7e4814263019dab099efa8e3e5e

Parity's trace according to etherscan is

[
  {
    "action": {
      "callType": "call",
      "from": "0xa529806c67cc6486d4d62024471772f47f6fd672",
      "gas": "0x2d6e28",
      "input": "0x7065cb480000000000000000000000001523e55a1ca4efbae03355775ae89f8d7699ad9e",
      "to": "0x269296dddce321a6bcbaa2f0181127593d732cba",
      "value": "0x0"
    },
    "blockHash": "0x89b7d3af787002056d804c650d89ed9b956b92918e0f2443d43e69b8f59f0eec",
    "blockNumber": 11495,
    "result": {
      "gasUsed": "0x64bd",
      "output": "0x"
    },
    "subtraces": 1,
    "traceAddress": [],
    "transactionHash": "0x427dfdbe8077d2901a51767672db3d4c37b7b7e4814263019dab099efa8e3e5e",
    "transactionPosition": 13,
    "type": "call"
  },
  {
    "action": {
      "callType": "call",
      "from": "0x269296dddce321a6bcbaa2f0181127593d732cba",
      "gas": "0x2cae73",
      "input": "0x5dbe47e8000000000000000000000000a529806c67cc6486d4d62024471772f47f6fd672",
      "to": "0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff",
      "value": "0x0"
    },
    "blockHash": "0x89b7d3af787002056d804c650d89ed9b956b92918e0f2443d43e69b8f59f0eec",
    "blockNumber": 11495,
    "result": {
      "gasUsed": "0xa9d",
      "output": "0x0000000000000000000000000000000000000000000000000000000000000001"
    },
    "subtraces": 1,
    "traceAddress": [
      0
    ],
    "transactionHash": "0x427dfdbe8077d2901a51767672db3d4c37b7b7e4814263019dab099efa8e3e5e",
    "transactionPosition": 13,
    "type": "call"
  },
  {
    "action": {
      "callType": "delegatecall",
      "from": "0x269296dddce321a6bcbaa2f0181127593d732cba",
      "gas": "0x2bf459",
      "input": "0x7d65837a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a529806c67cc6486d4d62024471772f47f6fd672",
      "to": "0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff",
      "value": "0x0"
    },
    "blockHash": "0x89b7d3af787002056d804c650d89ed9b956b92918e0f2443d43e69b8f59f0eec",
    "blockNumber": 11495,
    "result": {
      "gasUsed": "0x2aa",
      "output": "0x0000000000000000000000000000000000000000000000000000000000000001"
    },
    "subtraces": 0,
    "traceAddress": [
      0,
      0
    ],
    "transactionHash": "0x427dfdbe8077d2901a51767672db3d4c37b7b7e4814263019dab099efa8e3e5e",
    "transactionPosition": 13,
    "type": "call"
  }
]

The trace produced by go-ethereum is:

{
  "type": "CALL",
  "from": "0xa529806c67cc6486d4d62024471772f47f6fd672",
  "to": "0x269296dddce321a6bcbaa2f0181127593d732cba",
  "input": "0x7065cb480000000000000000000000001523e55a1ca4efbae03355775ae89f8d7699ad9e",
  "output": "0x",
  "gas": "0x2d6e28",
  "gasUsed": "0x64bd",
  "value": "0x0",
  "calls": [
    {
      "type": "CALL",
      "from": "0x269296dddce321a6bcbaa2f0181127593d732cba",
      "to": "0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff",
      "input": "0x5dbe47e8000000000000000000000000a529806c67cc6486d4d62024471772f47f6fd672",
      "output": "0x0000000000000000000000000000000000000000000000000000000000000001",
      "gas": "0x2cae73",
      "gasUsed": "0xa9d",
      "value": "0x0",
      "calls": [
        {
          "type": "DELEGATECALL",
          "from": "0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff",
          "to": "0x42b02b5deeb78f34cd5ac896473b63e6c99a71a2",
          "input": "0x7d65837a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a529806c67cc6486d4d62024471772f47f6fd672",
          "output": "0x0000000000000000000000000000000000000000000000000000000000000001",
          "gas": "0x2bf459",
          "gasUsed": "0x2aa",
        }
      ]
    }
  ]
}

The difference between the two traces are in the DELEGATECALL from and to fields. Both Parity and go-ethereum report the the DELEGATECALL 2 nestings deep (tx -> CALL -> DELEGATECALL). Parity however reports the DELEGATECALL's from field the same as the main transaction's to field (0x269296dddce321a6bcbaa2f0181127593d732cba). IMHO this is wrong, since we switched context in the intermediate CALL from the 0x269296dddce321a6bcbaa2f0181127593d732cba contract to the 0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff contract.

@karalabe karalabe changed the title Transaction traces seem to be wrong for delegatecall From/To in transaction traces seem to be wrong for delegatecall Nov 29, 2017
@5chdn
Copy link
Contributor

5chdn commented Nov 30, 2017

Might be related. #6087

@lastperson
Copy link

I confirm that parity traces was always behaving like this (incorrectly). It is not entirely wrong though. It copies from and to fields because that is how context looks like, as msg.sender is still the original caller and address(this) is still the same contract. So parity shows the context of the call instead of the actual call parameters. Actual call parameters is what expected here by the observer.

@karalabe
Copy link
Author

But the problem is that Parity displays the context 2 level up, not the correct one. For this particular transaction, we have

USER 0xa529806c67cc6486d4d62024471772f47f6fd672 ->
    CALL 0x269296dddce321a6bcbaa2f0181127593d732cba ->
        CALL 0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff ->
            DELEGATECALL 0x42b02b5deeb78f34cd5ac896473b63e6c99a71a2

So the from field when inside delegatecall should be 0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff since that was the outer context when entering, but Parity reports as 0x269296dddce321a6bcbaa2f0181127593d732cba, which is one CALL higher in the call stack.

@karalabe
Copy link
Author

Ah wait, I see what you're saying. The context is 0x269296dddce321a6bcbaa2f0181127593d732cba -> 0x13204f5d64c28326fd7bd05fd4ea855302d7f2ff, and that is what delegatecall retains. Yes, indeed I was wrong here.

Then again, you may again be right in that the Parity trace currently cannot discern the actual contract being delegatecalled. Interesting case :)

@5chdn 5chdn added F2-bug 🐞 The client fails to follow expected behavior. M6-rpcapi 📣 RPC API. P5-sometimesoon 🌲 Issue is worth doing soon. labels Jan 3, 2018
@5chdn 5chdn added this to the 1.9 milestone Jan 3, 2018
debris added a commit that referenced this issue Jan 15, 2018
debris added a commit that referenced this issue Jan 17, 2018
* Fixed delegatecall's from/to, closes #7166

* added tests for delegatecall traces, #7167
debris added a commit that referenced this issue Jan 22, 2018
* Fixed delegatecall's from/to, closes #7166

* added tests for delegatecall traces, #7167
debris added a commit that referenced this issue Jan 22, 2018
* Fixed delegatecall's from/to, closes #7166

* added tests for delegatecall traces, #7167
5chdn pushed a commit that referenced this issue Jan 23, 2018
* Update .gitlab-ci.yml

fix cache:key

* Fixed delegatecall's from/to (#7568)

* Fixed delegatecall's from/to, closes #7166

* added tests for delegatecall traces, #7167

* Fix Temporarily Invalid blocks handling (#7613)

* Handle temporarily invalid blocks in sync.

* Fix tests.

* Improve handling of RocksDB corruption (#7630)

* kvdb-rocksdb: update rust-rocksdb version

* kvdb-rocksdb: mark corruptions and attempt repair on db open

* kvdb-rocksdb: better corruption detection on open

* kvdb-rocksdb: add corruption_file_name const

* kvdb-rocksdb: rename mark_corruption to check_for_corruption
5chdn pushed a commit that referenced this issue Jan 23, 2018
* Improve handling of RocksDB corruption (#7630)

* kvdb-rocksdb: update rust-rocksdb version

* kvdb-rocksdb: mark corruptions and attempt repair on db open

* kvdb-rocksdb: better corruption detection on open

* kvdb-rocksdb: add corruption_file_name const

* kvdb-rocksdb: rename mark_corruption to check_for_corruption

* Hardening of CSP (#7621)

* Fixed delegatecall's from/to (#7568)

* Fixed delegatecall's from/to, closes #7166

* added tests for delegatecall traces, #7167

* Light client RPCs (#7603)

* Implement registrar.

* Implement eth_getCode

* Don't wait for providers.

* Don't wait for providers.

* Fix linting and wasm tests.

* Problem: AttachedProtocols don't get registered (#7610)

I was investigating issues I am having with Whisper support. I've
enabled Whisper on a custom test network and inserted traces into
Whisper handler implementation (Network<T> and NetworkProtocolHandler
for Network<T>) and I noticed that the handler was never invoked.

After further research on this matter, I found out that
AttachedProtocol's register function does nothing:
https://github.com/paritytech/parity/blob/master/sync/src/api.rs#L172
but there was an implementation originally:
99075ad#diff-5212acb6bcea60e9804ba7b50f6fe6ec and it did the actual
expected logic of registering the protocol in the NetworkService.

However, as of 16d84f8#diff-5212acb6bcea60e9804ba7b50f6fe6ec ("finished
removing ipc") this implementation is gone and only the no-op function
is left.

Which leads me to a conclusion that in fact Whisper's handler never gets
registered in the service and therefore two nodes won't communicate
using it.

Solution: Resurrect original non-empty `AttachedProtocols.register`
implementation

Resolves #7566

* Fix Temporarily Invalid blocks handling (#7613)

* Handle temporarily invalid blocks in sync.

* Fix tests.
@tjayrush
Copy link

Hi. I'm not able to discern whether or not anything changed in Parity's traces based on the above discussion. Was there an error in Parity's traces? If yes, does this mean that traces from blocks prior to this fix are now different than they were with earlier version. Saying it a different way: if I cached some traces from old versions of Partiy, should I clear my cache?

@debris
Copy link
Collaborator

debris commented Jan 28, 2018

Was there an error in Parity's traces?

Yes, there was.

if I cached some traces from old versions of Partiy, should I clear my cache?

Yes, you should

@lastperson
Copy link

lastperson commented Mar 23, 2018

@debris
Same issue applies for the callcode.
Example tx on mainnet: 0xb87b345fa76a1bcf44769115058dcc07f3acf96e2f47bd4bea960ad94cf5b433

@clowestab
Copy link

Led here from the release notes for 1.10.0

"We fixed DELEGATECALL" imo is an insanely unhelpful release note - it points here where no definitive conclusion was documented as to exactly what the issue was. Nowhere (apart from the code) is it explained what has changed and why.

I think this is pretty important in the context of tjayrush's comment. 3rd parties rely on responses from Parity. They are being changed. There data will now be inconsistent without clear explanation..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F2-bug 🐞 The client fails to follow expected behavior. M6-rpcapi 📣 RPC API. P5-sometimesoon 🌲 Issue is worth doing soon.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants