search

RGB-WG / rgb-std

RGB standard libs for WASM & low-level integrations (no FS/networking)
https://rgb.tech
Apache License 2.0
66 stars 31 forks source link

Avoid possible mismatches between witness txid and tx data at type system level #239

Closed dr-orlovsky closed 1 month ago

dr-orlovsky commented 1 month ago

This is to further strengthen the solution for https://github.com/RGB-WG/rgb-core/issues/261

https://github.com/RGB-WG/rgb-core/pull/262 ensures the match at the consensus level, but one may still create invalid consignments with fake data. This PR provides an ability for such invalid consignments to exist (at expense of some slightly higher computation requirements, which can be optimized later by hashing).

@St333p can't add you to the reviewers (IDNK why), but pls review this.

NB: This is a breaking change, thus old consignments and potentially old interfaces won't work; in order to test it you need to re-generate interfaces and schemata

zoedberg commented 1 month ago

I've tested this PR and its counterpart, they work fine.

Wondering though why there are TODOs like "Consider using UnsignedTx here" or "Add SPV as an option here". Wouldn't these changes affect the consignment structure? Since we are trying to make RGB stable and don't want to keep having breaking changes I would solve those TODOs before merging this.

While on this subject and in more general terms, I would look for all TODOs in the code and address them before the upcoming audit. If it helps I can open issues for each TODO I find so that we keep track of them on github.

@St333p can't add you to the reviewers (IDNK why), but pls review this.

I think because he's not a member of RGB-WG. Could you please add him there and to other RGB organizations?

dr-orlovsky commented 1 month ago

I've tested this PR and its counterpart, they work fine.

Does this mean I can merge it?

Consider using UnsignedTx here

This will require moving UnsignedTx from bp-std to bp-core, which I am hesitating to do.

Add SPV as an option here

It will be added as another enum variant

Since we are trying to make RGB stable and don't want to keep having breaking changes I would solve those TODOs before merging this.

All these todos are non-breaking in terms of neither consensus or consignment serialization structure. UnsignedTx has the exact same consensus serialization as Tx; it just enforces removal of witness and sigScript, which are not used in RGB anyhow. Adding another option to an enum doesn't affect byte structure, since an enum always reserve a byte for the variant tag, and we can add more variants not changing the serialization; it is forward-compatible. Adding it now will require writing a lot of code which postpone release further and can be easily done in v0.12: stabilization doesn't prevents new APIs from being added.

While on this subject and in more general terms, I would look for all TODOs in the code and address them before the upcoming audit. If it helps I can open issues for each TODO I find so that we keep track of them on github.

Thank you! This is a big work! Before we release v0.11 I expect todos to continue changing all around; so maybe the release will be the best time. I was revising all todos just before beta-6 and checked that none of them is breaking or a pushback (if we talk about RGB Core).

zoedberg commented 1 month ago

Does this mean I can merge it?

You can merge it.

About TODOs and auditing we will discuss this separetely.