Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in validating transition seals #55

Closed
crisdut opened this issue May 14, 2023 · 31 comments · Fixed by BP-WG/bp-core#43
Closed

Error in validating transition seals #55

crisdut opened this issue May 14, 2023 · 31 comments · Fixed by BP-WG/bp-core#43
Assignees
Labels
bug Something isn't working

Comments

@crisdut
Copy link
Member

crisdut commented May 14, 2023

Hi @dr-orlovsky,

I continued the consecutive transfer test and found strange behavior at this step.

The condition is only valid on the first transfer. In subsequent transfers, the outpoint of the seal is always None, causing the VerifyError::WitnessNotClosingSeal. This error occurs because the seal outpoints always None after the first transfer.

For Example, the in the list below, the first blindseal causes VerifyError::WitnessNotClosingSeal:

BlindSeal { method: TapretFirst, txid: WitnessTx, vout: Vout(0), blinding: 11375681452054571931 }
BlindSeal { method: TapretFirst, txid: Txid(Txid("1ab6073b14c4232f4fac45b32c71b3fabb5234b2af8eef4aa96f72da38f2bf4d")), vout: Vout(0), blinding: 15229399772929434069 }

If we comment the step, the consignment is validated correctly.

@crisdut crisdut added the bug Something isn't working label May 15, 2023
@crisdut
Copy link
Member Author

crisdut commented May 16, 2023

Hi @dr-orlovsky,

Currently, I think we need an extra step on the consign operation: Add in someplace of the consignment file the prev_out txid to the new transfer.

Today, the first transfer retrieves the information from Genesis->assignments, we need to add this information to validate the consignment file, we need all predecessor transitions, right?

This is one way to change WitnessTx to prev_out txid and pass in validation.

@dr-orlovsky
Copy link
Member

dr-orlovsky commented May 17, 2023

If we comment the step, the consignment is validated correctly.

It is not; it is just not validated, opening a vector for double-spending attack.

This step should work since it has to take Witness txid, i.e. construct an outpoint matching the witness transaction - and find it among inputs of the spending transaction. And the witness is constructed with the information coming from the anchor - which contains witness txid. Thus, we do not need to add anything anywhere.

https://github.com/RGB-WG/rgb-core/blob/94fd411bfe51039cc548dd5933d2c9af41637634/src/validation/validator.rs#L373C31-L376

The question remains though why the validation still fails not finding the input. Can you add dbg!(...) around self to inspect what's wrong with the witness construction

@crisdut
Copy link
Member Author

crisdut commented May 17, 2023

It is not; it is just not validated, opening a vector for double-spending attack.

Yes, yes, I know!

I commented on this snippet to make sure there not were other issues. =)

The question remains though why the validation still fails not finding the input. Can you add dbg!(...) around self to inspect what's wrong with the witness construction

In general, only genesis BlindSeal has the outpoint, the others only have "Ptr::WitnessTx", where this verification fails.

BTW, I will provide this information to you ASAP.

@dr-orlovsky
Copy link
Member

Not seal, witness structure is the one which has txid always. When a seal without txid happens it means that a txid from the anchor must be used - and that txid is transferred through Witness object.

@crisdut
Copy link
Member Author

crisdut commented May 17, 2023

Hi @dr-orlovsky,

Added a dbg! on BlindSeal too. See below:

[/home/crisdut/projects/contrib/rgb-core/src/validation/validator.rs:420] seal = BlindSeal {
    method: TapretFirst,
    txid: WitnessTx,
    vout: Vout(
        0,
    ),
    blinding: 11375681452054571931,
}
[/home/crisdut/projects/contrib/bp-core/seals/src/txout/witness.rs:97] self = Witness {
    tx: Tx {
        version: TxVer(
            2,
        ),
        inputs: Confined(
            [
                TxIn {
                    prev_output: Outpoint {
                        txid: Txid(
                            "9ab6b9837f9a681ecfbdeb6ec34ba002c42b23c6d43e5dd26fe0a56a726b869e",
                        ),
                        vout: Vout(
                            0,
                        ),
                    },
                    sig_script: SigScript(
                        ScriptBytes(
                            Confined(
                                [...],
                            ),
                        ),
                    ),
                    sequence: SeqNo(
                        4294967295,
                    ),
                    witness: Witness(
                        Confined(
                            [
                                Confined(
                                    [...],
                                ),
                            ],
                        ),
                    ),
                },
            ],
        ),
        outputs: Confined(
            [
                TxOut {
                    value: Sats(
                        9998000,
                    ),
                    script_pubkey: ScriptPubkey(
                        ScriptBytes(
                            Confined(
                                [...],
                            ),
                        ),
                    ),
                },
            ],
        ),
        lock_time: LockTime(
            0,
        ),
    },
    txid: Txid(
        "f7418e133dacad73b0d09f77c3c19cdcd486f2df2d30c1754a6b53c5cb85d909",
    ),
    proof: TapretFirst(
        TapretProof {
            path_proof: TapretPathProof {
                partner_node: None,
                nonce: 0,
            },
            internal_pk: InternalPk(
                XOnlyPublicKey(
                    53a1ec17b77db57ef39e76464555ebe9fbdfc779850b56074f040d249cdc46d5d4a06994078489cce5d654b62161ca5679af408536d7ee0716358803b93a14f1,
                ),
            ),
        },
    ),
}


[/home/crisdut/projects/contrib/rgb-core/src/validation/validator.rs:420] seal = BlindSeal {
    method: TapretFirst,
    txid: Txid(
        Txid(
            "1ab6073b14c4232f4fac45b32c71b3fabb5234b2af8eef4aa96f72da38f2bf4d",
        ),
    ),
    vout: Vout(
        0,
    ),
    blinding: 15229399772929434069,
}
[/home/crisdut/projects/contrib/bp-core/seals/src/txout/witness.rs:97] self = Witness {
    tx: Tx {
        version: TxVer(
            2,
        ),
        inputs: Confined(
            [
                TxIn {
                    prev_output: Outpoint {
                        txid: Txid(
                            "1ab6073b14c4232f4fac45b32c71b3fabb5234b2af8eef4aa96f72da38f2bf4d",
                        ),
                        vout: Vout(
                            0,
                        ),
                    },
                    sig_script: SigScript(
                        ScriptBytes(
                            Confined(
                                [],
                            ),
                        ),
                    ),
                    sequence: SeqNo(
                        4294967295,
                    ),
                    witness: Witness(
                        Confined(
                            [
                                Confined(
                                    [...],
                                ),
                            ],
                        ),
                    ),
                },
            ],
        ),
        outputs: Confined(
            [
                TxOut {
                    value: Sats(
                        9999000,
                    ),
                    script_pubkey: ScriptPubkey(
                        ScriptBytes(
                            Confined(
                                [...],
                            ),
                        ),
                    ),
                },
            ],
        ),
        lock_time: LockTime(
            0,
        ),
    },
    txid: Txid(
        "9ab6b9837f9a681ecfbdeb6ec34ba002c42b23c6d43e5dd26fe0a56a726b869e",
    ),
    proof: TapretFirst(
        TapretProof {
            path_proof: TapretPathProof {
                partner_node: None,
                nonce: 0,
            },
            internal_pk: InternalPk(
                XOnlyPublicKey(
                    53a1ec17b77db57ef39e76464555ebe9fbdfc779850b56074f040d249cdc46d5d4a06994078489cce5d654b62161ca5679af408536d7ee0716358803b93a14f1,
                ),
            ),
        },
    ),
}
Consignment is NOT valid
Validation failures:
- transition e9dbb0762f80b2b72bdd5789f1f25af3014fdd421e19a083f5c742b954270a00 doesn't close seal with the witness transaction f7418e133dacad73b0d09f77c3c19cdcd486f2df2d30c1754a6b53c5cb85d909. Details: the provided witness transaction f7418e133dacad73b0d09f77c3c19cdcd486f2df2d30c1754a6b53c5cb85d909 does not closes seal f7418e133dacad73b0d09f77c3c19cdcd486f2df2d30c1754a6b53c5cb85d909:0.
Validation warnings:
- duplicated terminal seal 8bNtrM7bEB6Ytb99jEibEA8FbZgzMqbJ9L36HrGf9W3o in operation e9dbb0762f80b2b72bdd5789f1f25af3014fdd421e19a083f5c742b954270a00.

@dr-orlovsky
Copy link
Member

Right... Very interesting.

The first transfer creates witness transaction spending 9ab6b9837f9a681ecfbdeb6ec34ba002c42b23c6d43e5dd26fe0a56a726b869e:0 output, which, I assume, contains the originally issued asset. The txid of this first witness transaction is f7418e133dacad73b0d09f77c3c19cdcd486f2df2d30c1754a6b53c5cb85d909. It also assigns all assets to the first (and the only) output of this transaction via WitnessVout seal.

Now, the second transfer tries to transfer assets not from that newly created assignment - but from 1ab6073b14c4232f4fac45b32c71b3fabb5234b2af8eef4aa96f72da38f2bf4d:0 (WTF is that and where it comes from?). Moreover, it creates a witness transaction with id 9ab6b9837f9a681ecfbdeb6ec34ba002c42b23c6d43e5dd26fe0a56a726b869e, which is not possible, since that was the original (already existing) transaction holding the very first assignment we were spending in the first transfer.

Unsuprisingly the validation fails.

Good news: validation works as expected and doesn't allow bullshit to pass.
Bad news: wallet producing this transaction and RGB std lib does this bullshit.

I.e. this is an application-level bullshit, not consensus-level, i.e. the issue is in rgb-std, rgb-wallet or bitmask-core - but not in bp-core or rgb-core.

Can you pls provide details about how this transfers were created? Can I expect transactions with that IDs? Are they in testnet or your regtest?

@dr-orlovsky
Copy link
Member

@crisdut I need more information from you here:

Can you pls provide details about how this transfers were created? Can I expect transactions with that IDs? Are they in testnet or your regtest?

You can also check the explanation of the problem I have given above. Right now it is not clear to me how to process with the issue without understanding how these tx ids are produced and what is in the actual transactions.

@dr-orlovsky dr-orlovsky transferred this issue from BP-WG/bp-core May 26, 2023
@dr-orlovsky dr-orlovsky transferred this issue from another repository May 26, 2023
@crisdut
Copy link
Member Author

crisdut commented May 26, 2023

Hi @dr-orlovsky,

I tested in Regtest.

My tests consist of issuing a contract with five amounts and trying to send it to the beneficiary in two parts, 1 per invoice.

  • The transaction 9ab6b9837f9a681ecfbdeb6ec34ba002c42b23c6d43e5dd26fe0a56a726b869e represents the first invoice paid.
  • The transaction f7418e133dacad73b0d09f77c3c19cdcd486f2df2d30c1754a6b53c5cb85d909 represents the second invoice paid.

I generate the second PSBT (transaction) from the change UTXO, using the "change" derivation path /1/0 + tweak (Tapret commitment). Because of this, the prev_out of the f7418e133dacad73b0d09f77c3c19cdcd486f2df2d30c1754a6b53c5cb85d909 is 9ab6b9837f9a681ecfbdeb6ec34ba002c42b23c6d43e5dd26fe0a56a726b869e.

@crisdut
Copy link
Member Author

crisdut commented Jun 1, 2023

Hi @dr-orlovsky,

Some updates:

I think I found the issue with consecutive transfers.

When we create a transfer in RGB, the bundle is add to the consignment file with BlindSeal->Txid set to WitnessTx.

The problem is that if the transition has a change assets, the rgb-wallet does not relate the TxId of the transfer with the BlindSeal of change. So when the user tries to spend this change, the system returns the VerifyError::WitnessNotClosingSeal error.

In this case, the bundle should be correctly updated in the transfer creation step. In this way, the acceptance stage would remain only to update the status of the contract.

PS: I managed to make a transfer with the change, but I was updating the bundles in the acceptance step and not in the transfer. Tomorrow I will try to do the operations in the transfer step.

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Jun 1, 2023

Sorry, I am failing to parse what you are saying...

the rgb-wallet does not relate the TxId of the transfer with the BlindSeal of change

What do you mean by "relate"? Wallet doesn't do that relations at all...

So when the user tries to spend this change, the system returns the VerifyError::WitnessNotClosingSeal error.

This error is generated not when we are trying to spend the change, but when we verify the spend.

In this case, the bundle should be correctly updated in the transfer creation step.

Correctly updated to what?

@crisdut
Copy link
Member Author

crisdut commented Jun 1, 2023

Ok, sure, I will try to clarify.

I believe that the problem of not being able to make asset transactions using the change from the previous asset transaction is related to the lack of information on the bundles.

I will separate my explanation into two parts: On the side of who creates the transaction (aka. owner) and on the beneficiary side.

In the current version of rgb-wallet, when an asset transaction is created and accepted, the beneficiary can use the received asset because, in the accept step, we use the secret seal to reveal the content of the bundles.

However, on the owner's side, we cannot use the change asset from the previous state transition because when we validate the consignment file, the system returns VerifyError::WitnessNotClosingSeal.

Yesterday I added one more step in the accept process, which allowed updating the owner's bundle containing the transaction information performed in L1. I managed to spend the owner's change after that.

But in the case of changing the owner, the bundle update should be in the transfer step itself in this piece of code.

Is it clearer now?

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Jun 1, 2023

However, on the owner's side, we cannot use the change from the previous transaction because when we validate the consignment file, the system returns VerifyError::WitnessNotClosingSeal.

Why would the owner have a need to use the output belonging to the receiver?

Is it clearer now?

Still struggling, especially with this part due to a non-standard terminology:
"In the current version of rgb-wallet, when a transaction is created and accepted"

What do you mean by transaction? Bitcoin witness transaction? State transition? Consignment? It looks like you are talking about consignment since the transaction can't be "accepted" - it is a property of the consignment. But why would the creator of the transfer consignment need to accept it? Can't compile

@crisdut
Copy link
Member Author

crisdut commented Jun 1, 2023

Why would the owner have a need to use the output belonging to the receiver?

To update the owner contract state in stock/inventory.

Still struggling, especially with this part due to a non-standard terminology:

I updated the content. I hope this help.

But in the case of changing the owner, the bundle update should be in the transfer step itself in this piece of code.

Anyway, I'll try to solve it, too, as mentioned above. I think we can talk better using Rust 😆😆😆😆

@dr-orlovsky
Copy link
Member

The sender should not accept the consignment it creates: it must be a no-op. All the data are already put to the stash when the consignment was created.

@dr-orlovsky
Copy link
Member

Ok, I have finally out figured this out. Highly untrivial bug.

It is caused by the fact that the validator needs to know a witness_txid for each of the inputs of the current state transition - which is different from the witness_txid for the current state transition. The code itself was providing the validator with the current one, not the previous one. This resulted that WitnessVout seals resolving to invalid txid and didn't pass the validation.

Bugfix required changes to both BP Core and RGB Core. Here are the PRs:

@crisdut
Copy link
Member Author

crisdut commented Jun 11, 2023

WOW, really?

I had noticed this difference between the TxID that was provided by the seals, sometimes the witness TxId, sometimes the current Txid.

I thought the problem lay in some bad construction of the transitions in the transfer step.

Unfortunately I didn't have time to dig deeper and find this, but I'm glad you did.

I will test ASAP and update you!

Thanks!

@dr-orlovsky
Copy link
Member

Yes, these multiple txid are really confusing and I was hunting them down for a week.

@zoedberg
Copy link
Contributor

I've tried to patch the dependencies but I'm encountering the following error:

   Compiling rgb-core v0.10.3 (https://github.com/RGB-WG/rgb-core?branch=fix/seal-vout#2295a90e)
error[E0308]: mismatched types
   --> /home/zoe/.cargo/git/checkouts/rgb-core-545e07908459752c/2295a90/src/validation/validator.rs:374:57
    |
374 |                 let witness = Witness::with(witness_tx, anchor.clone());
    |                               -------------             ^^^^^^^^^^^^^^ expected `Proof`, found `Anchor<MerkleProof>`
    |                               |
    |                               arguments to this function are incorrect
    |
    = note: expected enum `bp::dbc::Proof`
             found struct `bp::dbc::Anchor<MerkleProof>`
note: associated function defined here
   --> /home/zoe/.cargo/git/checkouts/bp-core-448b0ccbc7196e74/1c9b487/seals/src/txout/witness.rs:43:12
    |
43  |     pub fn with<L: mpc::Proof + StrictDumb>(tx: Tx, proof: Proof) -> Witness {
    |            ^^^^

error[E0609]: no field `txid` on type `bp::seals::txout::Witness`
   --> /home/zoe/.cargo/git/checkouts/rgb-core-545e07908459752c/2295a90/src/validation/validator.rs:388:28
    |
388 |         let txid = witness.txid;
    |                            ^^^^ unknown field
    |
    = note: available fields are: `tx`, `proof`

error[E0308]: `else` clause of `let...else` does not diverge
   --> /home/zoe/.cargo/git/checkouts/rgb-core-545e07908459752c/2295a90/src/validation/validator.rs:421:101
    |
421 |               let Some(prev_witness_txid) = self.anchor_index.get(&op).map(|anchor| anchor.txid) else {
    |  _____________________________________________________________________________________________________^
422 | |                 self.status.add_warning(Warning::AnchorNotFound(op));
423 | |             };
    | |_____________^ expected `!`, found `()`
    |
    = note:   expected type `!`
            found unit type `()`
    = help: try adding a diverging expression, such as `return` or `panic!(..)`
    = help: ...or use `match` instead of `let...else`

error[E0277]: the trait bound `Result<BlindSeal<bp::Txid>, bp::seals::txout::blind::TxidMismatch>: TxoSeal` is not satisfied
   --> /home/zoe/.cargo/git/checkouts/rgb-core-545e07908459752c/2295a90/src/validation/validator.rs:435:22
    |
435 |                     .verify_many_seals(&seals, &commitment)
    |                      ^^^^^^^^^^^^^^^^^ the trait `TxoSeal` is not implemented for `Result<BlindSeal<bp::Txid>, bp::seals::txout::blind::TxidMismatch>`
    |
    = help: the following other types implement trait `TxoSeal`:
              BlindSeal<Id>
              ExplicitSeal<Id>
    = note: required for `bp::seals::txout::Witness` to implement `single_use_seals::SealWitness<Result<BlindSeal<bp::Txid>, bp::seals::txout::blind::TxidMismatch>>`

Some errors have detailed explanations: E0277, E0308, E0609.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `rgb-core` due to 4 previous errors

@dr-orlovsky
Copy link
Member

@zoedberg it can be I forget to push from the other machine. Will be able to check bit later

@zoedberg
Copy link
Contributor

zoedberg commented Jun 13, 2023

@dr-orlovsky I've seen you force pushed the PRs, now code compiles but some send tests that previously worked are now failing with:

'anchor index is built from the seal sources so mismatch should be impossible: TxidMismatch { expected: Txid("07cc5fdf6364b01bbaf2cece1721441427a72134fccc48aa395b094c5819be98"), present: Txid("68119b68a8dc155eb3b69fff82c004bb5c72d12a33eb75413628790f5065b76d") }', ~/.cargo/git/checkouts/rgb-core-545e07908459752c/a5001f8/src/validation/validator.rs:424:64

Call throwing the error seems to be:

let transfer = bindle
    .unbindle()
    .validate(runtime.resolver())
    .unwrap_or_else(|c| c);

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Jun 14, 2023

@zoedberg I added this exception to see if the logic is invalid. And it seems to be invalid.
It is happening from here:

https://github.com/RGB-WG/rgb-core/pull/156/files#diff-5da346970c7ab1a20e8ab37567dd9b358c668ccfd6dccc2c7e6a187cebdd9c5aR424-R427

Can you provide more details on the failing test?

@dr-orlovsky
Copy link
Member

Ok, I have understood in which assumption I was wrong. Pushed to both repos. Also the footprint of the fix now is minimal. Can you pls check now?

@zoedberg
Copy link
Contributor

@dr-orlovsky Send tests that stopped working with the last update are now working again. While the send test that I think is related to this issue is still failing.

To add more info, the test does the following:

  1. Alice issues an asset and sends it to Bob, which receives it succesfully.
  2. Then Bob sends the asset back to Alice, which receives it succesfully.
  3. Then Alice tries to transfer the asset back again to Bob, but when Bob validates the consignment it fails with the following validation status:
Status{
  unresolved_txids: [
    
  ],
  unmined_terminals: [
    Txid("5744b2f70cfde3258eecec8d1fceb79452f6f92cc9cfeda69938b595de866039")
  ],
  failures: [
    ConfidentialSeal(Opout{
      op: OpId(Array<32>(fa702f90d9495c910938d7882a14f4118ff7f18d8a4b897db80a37cac0a31378)),
      ty: 2000,
      no: 1
    })
  ],
  warnings: [
    AnchorNotFound(OpId(Array<32>(3f6a0505f82d0bd35c7f593b6f2f8a18181d59ed85cb0d2d0a2c1c8abc8658f8))),
    TerminalWitnessNotMined(Txid("5744b2f70cfde3258eecec8d1fceb79452f6f92cc9cfeda69938b595de866039"))
  ],
  info: [
    
  ]
}

@dr-orlovsky
Copy link
Member

@zoedberg thank you for checking. It seems the failing test is not related to this issue but instead to #56

The problem of this issue was not with a seal being confidential but with seal witness txid validation.

@dr-orlovsky
Copy link
Member

@zoedberg this other problem has to be fixed by #63

@nicbus
Copy link
Contributor

nicbus commented Jun 14, 2023

I have tried the updated code in our regtest sandbox, which uses rgb-contracts (with patched dependencies and a quick fix to have a working issuance).

What I do to check for this issue is:

  1. issue an asset (2000)
  2. send some assets (100) from the issuer to a 1st recipient
  3. send some more assets (200) from the issuer to the same recipient
  4. send some assets (250) from the 1st recipient to a 2nd recipient

This way I check that the issuer is able to spend the change allocation, which is created via WitnessUtxo, and then that the resulting allocation is itself spendable.

At the moment, step 4 fails when I call rgb transfer with error:

Error: InsufficientState

Looking at consignment validation from step 3 I see the following warnings:

Validation warnings:
- duplicated terminal seal A8f...AAf in operation f06...4fa.
- anchor for operation 9cd...bdb is not found in the index. This is possible an internal bug of RGB Core library.
- terminal witness transaction 2ff...956 is not yet mined.

I'm using rgb-contracts with the following patched dependencies and respective commits:

  • aluvm 5f7c92a
  • bp-core (and bp-dbc, bp-primitives, bp-seals) fd1abc0
  • commit_verify 46cb8a7
  • rgb-core c2a4fef
  • rgb-schemata 692abac
  • rgb-wallet (and rgb-std) d5d40fc
  • strict_types 3505616

@dr-orlovsky
Copy link
Member

@nicbus this looks like a different bug from the originally reported in thus issue, isn't it?

I mean can I merge the PR fixing this issue and can you open a new one with this problem? ("Anchor absent in the index")

@nicbus
Copy link
Contributor

nicbus commented Jun 15, 2023

@nicbus this looks like a different bug from the originally reported in thus issue, isn't it?

I mean can I merge the PR fixing this issue and can you open a new one with this problem? ("Anchor absent in the index")

I'll open a new issue about the warning, but I'm not sure the InsufficientState error is related to this warning.

The reason for this is that I'm seeing the anchor for operation 9cd...bdb is not found in the index warning on all rgb accept invocations and on all rgb validate invocations, except for the call to validate during the first trasfer after issuance (but then rgb accept shows the warning).

@dr-orlovsky
Copy link
Member

I'll open a new issue about the warning, but I'm not sure the InsufficientState error is related to this warning.

It could be unrelated, so probably a dedicated issue is desirable. InsufficientState means that rgb can't coinselect enough inputs to cover the amount from the invoice.

Can you also attach a stash which fails in the test?

@dr-orlovsky
Copy link
Member

@nicbus btw do you confirm that this issue is closed and the bugfix works?

@nicbus
Copy link
Contributor

nicbus commented Jun 16, 2023

@nicbus btw do you confirm that this issue is closed and the bugfix works?

I confirm I don't see the original validation error transition 11d...dbf doesn't close seal with the witness transaction ca8...33c anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants