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

Fix force accept of invalid consignments #29

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

dr-orlovsky
Copy link
Member

Closes #24

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Oct 18, 2023
@dr-orlovsky dr-orlovsky added this to the v0.10.x milestone Oct 18, 2023
@zoedberg
Copy link
Contributor

I tried this PR on the sandbox branch mentioned in #24, and it seems this fix is not enough to solve the issue.
Looking to the code I noticed that unmined_terminals (the vector in Status that if not empty brings to Validity::UnminedTerminals) gets filled only in this piece of code:

for (operation, _) in &self.end_transitions {
    if let Some(anchor) = self.anchor_index.get(&operation.id()) {
        if let Some(pos) = self
            .status
            .failures
            .iter()
            .position(|f| f == &Failure::SealNoWitnessTx(anchor.txid))
        {
            self.status.failures.remove(pos);
            self.status
                .unresolved_txids
                .retain(|txid| *txid != anchor.txid);
            self.status.unmined_terminals.push(anchor.txid);
            self.status
                .warnings
                .push(Warning::TerminalWitnessNotMined(anchor.txid));
        }
    }
}

so only if a Failure::SealNoWitnessTx is found. But looking to Failure::SealNoWitnessTx documentation I've noticed it says witness transaction {0} is not known to the transaction resolver..

This explains why in the sandbox the validation status results in:

Consignment has non-mined terminal(s)
Non-mined terminals:
- e348c9b3d541ac5a4b1f4e796068352e20c8ef0b967ed771951771b7597150f4
Validation warnings:
- terminal witness transaction e348c9b3d541ac5a4b1f4e796068352e20c8ef0b967ed771951771b7597150f4 is not yet mined.

when we validate the consignment before broadcasting the TX, while after broadcasting the TX (not mining any block afterwards) the validation status becomes:

Consignment is valid

If I didn't misunderstand the meaning of Validity::UnminedTerminals this warning should be present also when the transaction is in the mempool but is unmined. If so I think this PR is not enough to close #24

@dr-orlovsky
Copy link
Member Author

Consignment is valid whenever resolver returns transaction given tx id. RGB has not concept of mempool. You just need to write resolver that wouldn't return tx until it is mined or has 3 confirmations - whatever the wallet policy is.

@zoedberg
Copy link
Contributor

I find naming a bit confusing then. Validity::UnminedTerminals makes the developer think that terminals have not been mined (which means they have no confirmations). Maybe a less ambiguous name would be UnresolvedTerminals. And Validity::UnresolvedTransactions would be less confusing if named UnresolvedNonTerminals.

@dr-orlovsky
Copy link
Member Author

dr-orlovsky commented Oct 21, 2023

We can't change names without breaking API, thus it must be an independent PR targeting v0.11. Thus this PR hsould be merged first even if we will change the names after.

@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

Merging #29 (581ebed) into master (680102a) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is 0.0%.

@@          Coverage Diff          @@
##           master    #29   +/-   ##
=====================================
  Coverage     0.0%   0.0%           
=====================================
  Files           8      8           
  Lines         817    824    +7     
=====================================
- Misses        817    824    +7     
Flag Coverage Δ
rust 0.0% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/bin/rgb/command.rs 0.0% <0.0%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dr-orlovsky dr-orlovsky merged commit 083d5f0 into master Oct 23, 2023
19 of 20 checks passed
@dr-orlovsky dr-orlovsky deleted the fix/force-accept branch November 14, 2023 09:49
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
No open projects
Development

Successfully merging this pull request may close these issues.

transfer accept succeeds even if transaction has not been confirmed
2 participants