nervosnetwork / fiber

19 stars 10 forks source link

Fix shutdown inputs error #53

Closed chenyukang closed 5 months ago

chenyukang commented 5 months ago

Seems we mixed commitment signed tx and shutdown tx here, it will make shutdown transaction use the same inputs from funding tx, since funding tx is committed, submit shutdown tx will get an error:

TransactionFailedToResolve: Resolve failed Unknown
contrun commented 5 months ago

Can you elaborate on the error encountered? Please give all the context of the transaction whose submission raises error TransactionFailedToResolve: Resolve failed Unknown (e.g. what the raw transaction data structure it is, what is the status of all the dependent cells). My intention for the function aggregate_partial_signatures_to_consume_funding_cell was always to create a valid witness to consume the funding cell (i.e. not to consume the inputs of funding transaction, but the funding transaction outputs). It seems to me the callers of aggregate_partial_signatures_to_consume_funding_cell, sign_tx_to_consume_funding_cell and another code snippet you modified are indeed consuming the funding transaction output (funding cell). I made the implicit assumption that the tx passed to aggregate_partial_signatures_to_consume_funding_cell has already used funding cell as its input. Please give raw transaction data structure to verify this. If this is not the case, we may need to modify the constructed tx, or change implementation of aggregate_partial_signatures_to_consume_funding_cell, and rename it to something like fill_in_inputs_and_witnesses_for_tx_to_consume_funding_cell (This name is bad. My point is to always modify the transaction inputs and witnesses instead of passing shutdown parameter. Because this is redundant, I always assume the input is funding cell).

quake commented 5 months ago

Can you elaborate on the error encountered? Please give all the context of the transaction whose submission raises error TransactionFailedToResolve: Resolve failed Unknown (e.g. what the raw transaction data structure it is, what is the status of all the dependent cells). My intention for the function aggregate_partial_signatures_to_consume_funding_cell was always to create a valid witness to consume the funding cell (i.e. not to consume the inputs of funding transaction, but the funding transaction outputs). It seems to me the callers of aggregate_partial_signatures_to_consume_funding_cell, sign_tx_to_consume_funding_cell and another code snippet you modified are indeed consuming the funding transaction output (funding cell). I made the implicit assumption that the tx passed to aggregate_partial_signatures_to_consume_funding_cell has already used funding cell as its input. Please give raw transaction data structure to verify this. If this is not the case, we may need to modify the constructed tx, or change implementation of aggregate_partial_signatures_to_consume_funding_cell, and rename it to something like fill_in_inputs_and_witnesses_for_tx_to_consume_funding_cell (This name is bad. My point is to always modify the transaction inputs and witnesses instead of passing shutdown parameter. Because this is redundant, I always assume the input is funding cell).

this error may be triggered by e2e test, ref: https://github.com/contrun/ckb-pcn-node/actions/runs/9434095793/job/25985824223#step:6:1769

[2024-06-09T05:02:06Z ERROR ckb_pcn_node::ckb_chain::actor] [ckb-chain] send transaction failed: Rpc(Error { code: ServerError(-301), message: "TransactionFailedToResolve: Resolve failed Unknown(OutPoint(0x1bbdf535281ade12428e33e7a5839d1c0b52da66f5cf853c01f703874447826700000000))", data: Some(String("Resolve(Unknown(OutPoint(0x1bbdf535281ade12428e33e7a5839d1c0b52da66f5cf853c01f703874447826700000000)))")) }) [node 3]
[2024-06-09T05:02:06Z ERROR ckb_pcn_node::ckb_chain::actor] [ckb-chain] send transaction failed: Rpc(Error { code: ServerError(-301), message: "TransactionFailedToResolve: Resolve failed Unknown(OutPoint(0x1bbdf535281ade12428e33e7a5839d1c0b52da66f5cf853c01f703874447826700000000))", data: Some(String("Resolve(Unknown(OutPoint(0x1bbdf535281ade12428e33e7a5839d1c0b52da66f5cf853c01f703874447826700000000)))")) }) [node 1]
contrun commented 5 months ago

There was an overlook in https://github.com/contrun/ckb-pcn-node/pull/39 in which I copied some of the old code to build a transaction to consume the funding cell (including commitment transactions and shutdown transaction). This created an wrong transaction. I fixed this error in https://github.com/contrun/ckb-pcn-node/pull/57/.

It seems that the error in building commitment transaction was not caught earlier because the transaction verifier of ckb-testtool only considers whether the scripts in the transaction can run correctly and it is OK for a transaction to have already consumed inputs. Moreover, the error in e2e test was not correctly captured in CI (only showing the error in log).

This PR is closed in favor of https://github.com/contrun/ckb-pcn-node/pull/57/