Revision of v0.11.1 increasing code reliability

[dr-orlovsky]

This makes 0.11.1 smaller and more audible, removing unneeded dependencies, not-anymore-used code, functionality and getting rid of high-risk code, significantly reducing attack surface.

Key highlights:

• No more transition bundles;
• No more AluVM;
• No more bp-core crate (just bp-consensus and simplified bp-seals);
• No more anchors;
• No more Rc/RcRefs in validator, saving several clones and increasing performance;
• No more access to the contract global state (just operation-defined one);
• No more tapret seals;
• No more consensus ordering of neither global state or operations;
• No more injections of "trusted" data into the consensus level;
• No more complex merkle-based commitments to the operation data taking a lot of manual code;
• No more unused or excessive wrapper data types.

[codecov]

Codecov Report

Attention: Patch coverage is 0.98361% with 302 lines in your changes missing coverage. Please review.

Project coverage is 10.4%. Comparing base (bbc63f8) to head (bda74af).

Files with missing lines	Patch %	Lines
src/validation/validator.rs	0.0%	108 Missing ⚠️
src/validation/logic.rs	0.0%	95 Missing ⚠️
src/validation/util.rs	0.0%	51 Missing ⚠️
src/operation/assignments.rs	0.0%	23 Missing ⚠️
src/bin/rgbcore-stl.rs	0.0%	4 Missing ⚠️
src/operation/global.rs	20.0%	4 Missing ⚠️
src/schema/schema.rs	0.0%	4 Missing ⚠️
src/operation/state.rs	0.0%	3 Missing ⚠️
src/operation/commit.rs	0.0%	2 Missing ⚠️
src/operation/data.rs	0.0%	2 Missing ⚠️
... and 5 more

❌ Your patch check has failed because the patch coverage (1.0%) 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            @@
##           v0.11.1    #292     +/-   ##
=========================================
+ Coverage      9.8%   10.4%   +0.6%     
=========================================
  Files           27      23      -4     
  Lines         2670    1785    -885     
=========================================
- Hits           262     185     -77     
+ Misses        2408    1600    -808     

Flag	Coverage Δ
rust	10.4% <1.0%> (+0.6%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dr-orlovsky]

@codecov-ai-reviewer review

[codecov-ai]

On it! We are reviewing the PR and will provide feedback shortly.

[codecov-ai]

PR Description

This pull request refactors the RGB Core library to simplify its architecture, improve maintainability, and enhance security by removing the AluVM-based smart contract execution environment and replacing it with a declarative validation approach using Vesper. It also updates dependencies and streamlines data structures for improved efficiency.

Click to see more

Key Technical Changes

• Removed the aluvm dependency and related code for smart contract execution.
• Introduced a Verifier enum in src/schema/operations.rs to define declarative validation rules (EqSums, EqVals, CheckSigEcdsa).
• Simplified state handling by removing the RevealedState enum and directly using FungibleState and StructuredData.
• Refactored the commitment model, removing the TransitionBundle and related structures.
• Updated the rgbcore-stl binary to generate strict type libraries without logic-related types.
• Modified the validation process to use the new Verifier enum and simplified state access patterns.

Architecture Decisions

• Replaced the AluVM-based smart contract execution with a declarative validation system based on Vesper for improved security and reduced complexity.
• Simplified the state model to directly use FungibleState and StructuredData, removing the need for a separate RevealedState enum.
• Removed the concept of TransitionBundles and instead focused on individual transitions, simplifying the validation process.

Dependencies and Interactions

• Removes dependencies on aluvm, bp-dbc, single_use_seals, and other related crates.
• Updates dependencies on amplify, baid64, strict_encoding, strict_types, commit_verify, bp-consensus, bp-seals, and secp256k1.
• Introduces a patch for bp-seals to use a specific revision from GitHub.
• Interacts with the bc crate for blockchain primitives and seals crate for seal types.

Risk Considerations

• The removal of AluVM-based smart contract execution may impact existing contracts that rely on its functionality. A migration strategy or compatibility layer may be needed.
• The new declarative validation approach may require careful review and testing to ensure that all validation rules are correctly implemented and that no security vulnerabilities are introduced.
• The patch dependency on bp-seals from GitHub introduces a potential risk if the specified revision is not stable or contains bugs. It is recommended to switch to an official release on crates.io as soon as possible.
• The simplification of state handling may limit future extensibility if different state types or more complex state management are required.

Notable Implementation Details

• The Verifier enum in src/schema/operations.rs defines the different validation rules that can be applied to transitions.
• The validate_logic function in src/validation/logic.rs implements the validation logic based on the Verifier enum.
• The rgbcore-stl binary has been updated to generate strict type libraries without logic-related types.
• The TransitionBundle and related structures have been removed, and the validation process now focuses on individual transitions.

[dr-orlovsky]

@codecov-ai-reviewer got confused: vesper is not used in any validation. As before it is used just in producing docs on commitment structures, and was moved into a separate feature to make the dependency tree lighter when str feature is used downstream. Again, this has nothing to do with AluVM and other changes.

[zoedberg]

Could you please open a PR targeting zoedberg/0.11.1-3 on rgb-tests in order to show that existing features are still supported?

[dr-orlovsky]

Sure, but I do not see such a branch here. Is it this one? zoedberg#3

It seems somehow it got also two merges which I did for your branches from here...

[zoedberg]

I meant this branch https://github.com/zoedberg/rgb-tests/tree/0.11.1-3

[dr-orlovsky]

Hm, but that would require rgb-std and rgb changes where I would need your help.

Before processing with them can you at least concept and approach ack here?

[zoedberg]

We haven’t yet reviewed the code deeply enough to provide an N/ACK. We thought that updating rgb-tests was a quick way to first of all be sure no tested features have been removed.
The changes seem pretty invasive and the highlights alone don’t provide enough insight into the rationale behind them.
In order to avoid spending too much time on the review, please expand on the highlights giving rationales for the changes and any additional note you deem meaningful to understand them.
We’ll then be able to provide a N/ACK on the concept.

[dr-orlovsky]

Ok, here it is:

Rational

Fix double spending bugs and backdoors you have introduced into the consensus code. Described here: #288 (review)

I am doing your work for you.

Will this work?

[fedsten]

Ok, here it is:

Rational

Fix double spending bugs and backdoors you have introduced into the consensus code. Described here: #288 (review)

I am doing your work for you.

Will this work?

You have to spend your time understanding the system-level things, otherwise you spaghetti shitty code will be never merged or released.

@dr-orlovsky could you please expand your comment to properly explain the rationale behind your changes? Your PR introduces significant modifications and appears to remove several features currently used by existing wallets. If you're seeking a concept ACK, it's important to explain why the specific changes are necessary. If you don't care about receiving an ACK, then it's unclear why you're asking for one in the first place.

[dr-orlovsky]

This PR must not contain any changes which reduce known wallet or asset use of RGB. If you see any of the changes which can do that (even potentially) please let me know - and I will either explain how that functionality is in fact preserved, or, if you were right, will bring back missing functions.

Regarding the rational, it seems we just misunderstand each other. There is no other rational here than the one I already described:

1. remove known issues in v0.11.1 introduced by breaking consensus logic with extra-consensus "injections";
2. remove the rest of all unused, untested things in RGB v0.11.1 in order to reduce the attack surface as much as possible.

I am willing to adopt these changes in rgb-std, rgb and rgb-schemata on top and demonstrate that we will have the needed functionality - but before investing time into that I need to be sure that you are conceptually OK with this approach. "Conceptually" includes "yeah we are fine unless it leads to reduction in functionality of existing software/assets using RGB" (which I can and will fix, if it is the case).

[zoedberg]

This PR must not contain any changes which reduce known wallet or asset use of RGB. If you see any of the changes which can do that (even potentially) please let me know - and I will either explain how that functionality is in fact preserved, or, if you were right, will bring back missing functions.

There are several features that we are not sure are preserved. Let's start from the first that concerns us the most: by removing the TransitionBundle it seems you'd lose the ability of grouping multiple transitions of a given contract on a single witness. This could lead to problems if you end up with allocation of different types on a given UTXO, how would you avoid burning any of them?

[dr-orlovsky]

if you end up with allocation of different types on a given UTXO, how would you avoid burning any of them?

There are several ways how this can be achieved.

First, a contract schema may allow to spend or allocate multiple types in a transition.

So, for RGB20 schema which supports secondary inflation the "inflate" transition may be allowed to spend "owned" allocations.

Second option is to provide a dedicated transition type in a schema to unwind such conditions. This is not the same as a "blank transition"; the thing will be only used by the issuer if he got into this situation.

In all existing RGB2x standards the only party who can get several different types of assignments on the same UTXO is the asset issuer - and it will have tools to unwind that with both of approaches. The users are unaffected anyway.

[zoedberg]

Re-introducing a transition that allows to spend or allocate multiple (unrelated) types in a transition seems like a regression, even if not called "blank transitions" the proposed solutions would have similar issues.
To prevent inflation while handling multiple allocations on the same UTXO they'd need to be very strict and end up requiring lots of transition types or multiple TXs (1 to split, 1 to actually send), which seems very inefficient.

Also, notably, it's not only issuers who could end up in this situation, as right transfers are possible.

[dr-orlovsky]

Re-introducing a transition that allows to spend or allocate multiple (unrelated) types in a transition seems like a regression, even if not called "blank transitions" the proposed solutions would have similar issues.

First, it is not a regression: none of functionality is lost. The purpose is reduction of the attack surface: using bundles at the consensus level is an overkill for this!

To prevent inflation while handling multiple allocations on the same UTXO they'd need to be very strict and end up requiring lots of transition types or multiple TXs (1 to split, 1 to actually send), which seems very inefficient.

Sorry, I do not understand. The only thing you need is a transition type known only for issuer, which does the same thing as normal asset transfer, plus ensures that the sum of inflation inputs is equal to the sum of inflation outputs. Literally, one line of code.

Also, notably, it's not only issuers who could end up in this situation, as right transfers are possible.

When an issue right is transferred, the receiving party becomes an issuer by definition. Thus, it is only issuers who are affected.

[zoedberg]

it is not a regression: none of functionality is lost

It is. In order to conceal transitions (and thus increase privacy) we need to be able to define multiple transitions.

The only thing you need is a transition type known only for issuer, which does the same thing as normal asset transfer, plus ensures that the sum of inflation inputs is equal to the sum of inflation outputs. Literally, one line of code.

From the info you provided it's unclear what exactly you're suggesting. Would you mind providing the line of code that would achieve this?

When an issue right is transferred, the receiving party becomes an issuer by definition. Thus, it is only issuers who are affected.

There will not be just issue rights, also other kind of rigths exist and may be transferred to non-issuers as well.

[dr-orlovsky]

It is. In order to conceal transitions (and thus increase privacy) we need to be able to define multiple transitions.

I have no idea what do you mean by that. Which privacy increase? v0.11.1 before this version provides no privacy with bundles.

From the info you provided it's unclear what exactly you're suggesting. Would you mind providing the line of code that would achieve this?

This line of code will exist in a PR I will make in rgb-std once this will be complete/merged. If you can't see how to do it, I will do it for you there.

There will not be just issue rights, also other kind of rigths exist and may be transferred to non-issuers as well.

It is clear that the holders of this rights are singular parties designated by the issuer - and clearly not the common users of the assets or contracts.

Overall, I am tired of this arguing just for the sake of arguing. If you do not want to pay attention to my suggestions and fixes, just say that directly and stop wasting my time.

[St333p]

I have no idea what do you mean by that. Which privacy increase? v0.11.1 before this version provides no privacy with bundles.

#293 brings back the ability to conceal transitions in a bundle, we were already working on it and we didn't want to roll it back.

It is clear that the holders of this rights are singular parties designated by the issuer - and clearly not the common users of the assets or contracts.

I still don't see why we should remove the possibility of multiple transitions in a bundle. It reduces the toolset available for allocation management and it prevents concealed transitions, while not causing any (known) issues.

Overall, I am tired of this arguing just for the sake of arguing. If you do not want to pay attention to my suggestions and fixes, just say that directly and stop wasting my time.

I paid enough attention to be able to say that at least some of them go in an undesired direction. Also, it doesn't seem to me the right moment for such a deep consensus rework, we are aiming towards more targeted bugfixes and a release as soon as possible.