Refactoring v0.11 into v0.11.1 in style of v0.12 simplifications #288
Conversation
testnet may have blocks which timestamps are not aligned with their heights
fmt keeps changing all the time. Now they introduced new "editions" feature, which by default does a lot of changes to the codebase by mingling with alphabetical sorting of all imports, doing them in case-insentive way. This PR allows to avoid that dramatic changes
due to rustfix dependency
Co-authored-by: Stefano Pellegrini <[email protected]>
Release v0.11.0 beta 9
0.11.1-alpha.2
dr-orlovsky
changed the title
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (10.5%) is below the target coverage (60.0%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #288 +/- ##
=======================================
- Coverage 13.2% 9.8% -3.4%
=======================================
Files 29 27 -2
Lines 3883 2670 -1213
=======================================
- Hits 513 262 -251
+ Misses 3370 2408 -962
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| chain_net: ChainNet, | ||
| context: S::Context<'_>, | ||
| safe_height: Option<NonZeroU32>, | ||
| trusted_op_seals: BTreeSet<OpId>, |
There was a problem hiding this comment.
This introduces a backdoor into the RGB consensus.
This method is called only externally, exposing an access to the consensus verification for the application level - which is used on top to hide the double-spent issue also introduced in the consensus (see my next comment)
There was a problem hiding this comment.
trusted_op_sealshas nothing to do with the double-spend issue on the modified input map you are claiming- It's not a backdoor. If a wallet is compromised, it may ignore the consignment validation result or avoid running validation altogether. Allowing it to specify a set of opids it considers to be trusted (maybe because it validated them already, or for whatever reason) changes nothing in the security model
- It's currently used to handle replace transitions in IFA assets: a wallet may flag the transition as trusted because it involves the replace right of a issuer's delegate. This way, we can strip the history before the replace operation from consignment data. If you know of other ways to provide the same feature we can discuss the removal of
trusted_op_seals- A wallet still has the ability to not trust the issuer if the history behind replace operations gets published somewhere, by reconstructing the complete consignment and validating with an empty set of trusted seals
- It might also be useful for wallets to implement a sort of cache: store the opids of transitions that were validated so that their validation can be skipped if they appear in the history of multiple incoming transfers. This cache is not a killer feature though, if it ends up being the only reason why we have
trusted_op_sealsI'm ok with dropping it
There was a problem hiding this comment.
It was not me who invented separation of concerns - or a difference of consensus from non-consensus things.
This change clearly breaks both of matters. For zero reason. Not acceptable.
There was a problem hiding this comment.
For zero reason
I explained the reason. Until we have an alternative solution or we find issues/attacks I tend to be against dropping it
| serde(crate = "serde_crate", transparent) | ||
| )] | ||
| pub struct InputMap(Confined<BTreeMap<Vin, OpId>, 1, U16MAX>); | ||
| pub struct InputOpids(NonEmptyOrdSet<OpId, U16MAX>); |
There was a problem hiding this comment.
This change introduce a double-spending issue into the consensus, and doesn't make any sense.
-
if you require all transitions to be present, there is no need for the TransitionBundle at all: it adds nothing. Also, you kill the multi-party transaction use case privacy: payjoins, coinjoins, Ark etc - and will need a new structure next to PSBT and a more complex interactive process to get RGB working there
-
as of now, you do not require in cosensus for all transactions presence, thus, you have a double-spent attack in v0.11.1
There was a problem hiding this comment.
The validation code makes sure the double spend is not possible. This change allows to move different allocation types on a single UTXO, each with their transition.
you kill the multi-party transaction use case privacy
We have some work going on to re-introduce that feature.
you do not require in cosensus for all transactions presence
This is not true, we also did a test to prove that https://github.com/St333p/rgb-tests/blob/24ee570715c7e18d5ede7aa5ed38fa305c2b0086/tests/transfers.rs#L2258
There was a problem hiding this comment.
This validation code makes TranstionBundle redundant and not used, introducing additional complexity and attack vector, hidden by other parts of the code (which may change in the future, and this will be forgotten).
There was a problem hiding this comment.
TransitionBundle is still useful, there are cases in which one needs to bundle transitions of the same contract (e.g. allocations of different types on a single UTXO). Also, the ability to omit transitions from the bundle can be brought back safely by changing the input_map structure to Map<Opout, OpId>, we're currently working on this.
There was a problem hiding this comment.
TransitionBundle is still useful, there are cases in which one needs to bundle transitions of the same contract (e.g. allocations of different types on a single UTXO).
No, it is not: #292 (comment)
| if self.trusted_op_seals.contains(&opid) { | ||
| continue; | ||
| } | ||
| let Some(outpoints) = input_map.get(&opid) else { |
There was a problem hiding this comment.
Here, the known_transitions field of the bundle is not checked at all!
Instead, everything agains double-spent is relied onto a self.trusted_op_seals, injected as a backdoor from the application level!
There was a problem hiding this comment.
The double spend is prevented by the following check:
let Opout { op, ty, no } = input;
if !self.input_assignments.borrow_mut().insert(input) {
self.status
.borrow_mut()
.add_failure(Failure::DoubleSpend(input));
}together with the fact that all transitions in the input_map are effectively required to be in the known_transitions by the check at line 419 (variable input_map is built from bundle's known_transitions).
The self.trusted_op_seals is not there as a protection from double spends. Its purpose is to allow a wallet to skip validation of certain seals. It could be misused, the same as a wallet can run modified RGB libraries to do whatever it wants, but sane wallets will reject the transfer if invalid. For details about that see #288 (comment)
3 participants
This PR by Bitfinex team summarizes their 4-month work on making v0.11 as simple as possible, including taking many ideas from v0.12 (seal unification, monimorphisation of contracts over specific underlying chain; state unification, removal of Pedersen commitments etc)