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

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

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

dr-orlovsky
Copy link
Member

@dr-orlovsky dr-orlovsky commented Jul 24, 2024

This is to further strengthen the solution for RGB-WG/rgb-core#261

RGB-WG/rgb-core#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

@dr-orlovsky dr-orlovsky added the enhancement New feature or request label Jul 24, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Jul 24, 2024
@dr-orlovsky dr-orlovsky requested a review from zoedberg July 24, 2024 11:26
@dr-orlovsky dr-orlovsky self-assigned this Jul 24, 2024
@zoedberg
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

Does this mean I can merge it?

You can merge it.

About TODOs and auditing we will discuss this separetely.

@dr-orlovsky dr-orlovsky merged commit afe14d3 into master Jul 25, 2024
19 checks passed
@dr-orlovsky dr-orlovsky deleted the txid branch September 5, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants