Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 18.3%. Comparing base (c00aa3b) to head (aaeeade).

@@           Coverage Diff            @@
##           master    #288     +/-   ##
========================================
- Coverage    18.3%   18.3%   -0.0%     
========================================
  Files          38      38             
  Lines        8711    8719      +8     
========================================
  Hits         1598    1598             
- Misses       7113    7121      +8     

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

@zoedberg sorry, I do not get it.

Contract state must include ALL state, even the one belonging to witnesses which were excluded from the blockchain (in case they will be included back).

Thus, this PR is completely valid: it displays to the user filtered state only for those witnesses which are mined - but ignores (yet still keeping) the rest.

This PR is unrelated to the inflation bug you mentioned and fixes other issue: displaying of excessive state in a user UI. This state can't be spent as well, thus it solves an issue where a user unwillingly creates invalid consignment - or unable to create a consignment at all.

Accepting an invalid consignment is a different issue and should be addressed separately.

Now, on accepting invalid consignment.

Witness resolvers are created with a purpose to delegate decision what is a valid status for a witness transaction, and what is not - because this differs for different scenarios, like lightning state channel (where only the latest commitment transaction is a valid one, - but unmined - and the rest and unmined and invalid, but need to be kept).

From my understanding, "accepting invalid consignment" you refer to, does happen due to invalid witness resolver you provide the wallet with.

Now, we are coming to the question "what is a valid witness resolver for onchain transfer?" This question I have asked you on the last call, and was not able to explain what I mean. This exactly what I mean:

• if a valid witness resolver should not accept mempool transactions, than there is noway to validate a consignment before a tx is mined, and a receiver won't be able to confirm sender the validity before sender mines it;
• if it should accept mempool transaction, we are exactly in a situation where a re-org or an RBF later would leave receuiver with no funds.

I do agree that is what has to be delegated to the wallet and receiver to decide - but this is the exact behaviour of the system now: a wallet defines that by using a proper witness resolver, aligned with its policies.

For instance, it may use one witness resolver (accepting mempool transactions) for validation and confirmation to the sender, and than some other - for accepting into the stash consignment later.

it displays to the user filtered state only for those witnesses which are mined

State is not only a matter of "display", it's used to select rgb inputs when performing a send. Therefore with this PR we are removing the possibility of spending an rgb input associated to a witness TX that is in mempool (i.e. not mined). Moreover, the "invalid consignment" bug could already be solved by calling update_witnesses before retrying the transfer, fixing it without restricting the set of supported features.

From my understanding, "accepting invalid consignment" you refer to, does happen due to invalid witness resolver you provide the wallet with.

Looking at the test code, it can be seen that it's using a normal "online" indexer (i.e. esplora), which is a "valid" resolver for the scenario.
Moreover, even assuming the test was using a custom resolver (which it is not), I think the validation code should detect that an assignment is spent by 2 different transitions, thus causing inflation (issued amount is 2000 but after the inflation wallets start transferring and accepting 2100 units).

Therefore with this PR we are removing the possibility of spending an rgb input associated to a witness TX that is in mempool (i.e. not mined).

Ok, this makes sense, but that is why I have asked "what to put there". We kinda agreed it should be delegated to a wallet, correct? I mean to add an function call argument specifying minimal depth

Moreover, the "invalid consignment" bug could already be solved by calling update_witnesses before retrying the transfer, fixing it without restricting the set of supported features.

This part I do not get. If it could be solved by calling update_witness why didn't you add that call to the failing test? In other words, what do you propose to do here?

Looking at the test code, it can be seen that it's using a normal "online" indexer (i.e. esplora), which is a "valid" resolver for the scenario.

This brings us to the question I have asked:

Now, we are coming to the question "what is a valid witness resolver for onchain transfer?" This question I have asked you on the last call, and was not able to explain what I mean. This exactly what I mean:

• if a valid witness resolver should not accept mempool transactions, than there is noway to validate a consignment before a tx is mined, and a receiver won't be able to confirm sender the validity before sender mines it;
• if it should accept mempool transaction, we are exactly in a situation where a re-org or an RBF later would leave receiver with no funds.

So what a normal "online" indexer should do? It seems we need different "normal online" indexers for different cases, aren't we?

Moreover, even assuming the test was using a custom resolver (which it is not), I think the validation code should detect that an assignment is spent by 2 different transitions, thus causing inflation (issued amount is 2000 but after the inflation wallets start transferring and accepting 2100 units).

In v0.11 RGB consensus validation doesn't check double-spents and delegates that to bitcoin blockchain - as we discussed multiple times. This was changed in v0.12, which does the check. Adding the check to v0.11 impossible due to architectural reasons. Basically, v0.12 IS v0.11 with double-spend consensus checking in terms of consensus (plus field elements).

Ok, this makes sense, but that is why I have asked "what to put there". We kinda agreed it should be delegated to a wallet, correct? I mean to add an function call argument specifying minimal depth

I think it would be useful for the sender to include only assignments that have at least X depth, where X is decided by the wallet. This PR is just setting X=1, but IMO this is a new feature and is unrelated to the issues reported in #287 so 1) it is incorrect to say that this PR closes #287 and 2) I don't think it's a good idea to do this now.

This part I do not get. If it could be solved by calling update_witness why didn't you add that call to the failing test? In other words, what do you propose to do here?

If we add update_witness the test becomes the same as the same_transfer_twice_update_witnesses, while the same_transfer_twice_no_update_witnesses is there to show that if a sender creates an invalid consignment the receiver still accepts it and successive receivers thinks there are 2100 units of the asset even if only 2000 have been actually issued.

So what a normal "online" indexer should do? It seems we need different "normal online" indexers for different cases, aren't we?

The resolver/indexer is not the issue here. The validation code is the issue, since it doesn't detect an invalid consignment (one where there are 2 different transitions spending the same RGB assignment), and validation should be detecting that even without calling indexers altogether.

In v0.11 RGB consensus validation doesn't check double-spents and delegates that to bitcoin blockchain - as we discussed multiple times. This was changed in v0.12, which does the check. Adding the check to v0.11 impossible due to architectural reasons. Basically, v0.12 IS v0.11 with double-spend consensus checking in terms of consensus (plus field elements).

I don't agree that in v0.11 it's impossible to prevent this. Please check RGB-WG/rgb-core@6cd46eb.

TL;DR

1. the main issue is that recipients accept a consignment double-spending an allocation
2. double-spend needs to be prevented on the receiver side and it can be done in v0.11