no more possible to force accepting a transfer

[zoedberg]

With beta 5 it was possible to accept a transfer with the force boolean, with beta 6 this possibility has been removed. @dr-orlovsky I don't recall this being communicated or discussed. This breaks our Lightning Network node so please re-introduce this ASAP (to be included in the next beta release).

[dr-orlovsky]

This was not intentional. Can you provide a specific information which function signature is not there anymore?

[zoedberg]

During this commit range

git log --oneline add376c^..813bbcb

813bbcb containers: better differentiate Contract and Transfer types
8904684 persistence: impl display for consignments
24a4290 persistence: expose more methods
7f25ac1 persistence: expose type system in builder
705ea62 persistence: expose missed methods
10c4944 persistence: add debug getters for memory types
30a6b65 persistence: make load/store trait-based and generic
5159f80 persistence: implement file load/save operations
8183ba6 persistence: improve Stock error convertors
ef0df69 persistence: always resolve to mempool in fascia
aa75a57 persistence: return validation status from Stock::consume_consignment
5e0128d persistence: default implementations
43e1040 persistence: fix stock module
9251967 iface: bring back amount change
8814cf3 resolvers: relax Error trait requirements
f96f418 containers: remove outdated tests
2a8b656 persistence: provide default generics for Stock
add376c persistence: completed index, state and stock refactoring

the accept_transfer method has been removed (in add376c) and then re-added (in 813bbcb), but when it has been re-added its force boolean parameter has been dropped.

[dr-orlovsky]

Ok, I see now: the new methods take ValidConsignment (or ValidTransfer) and thus do not need force argument, since they do not try to validate anyway - there is nothing to force. And there should be unsafe method assume_valid to convert any consignment into valid version if needed

[nicbus]

there should be unsafe method assume_valid to convert any consignment into valid version if needed

I have looked into the code but couldn't find the assume_valid method. Can you be more specific, please?

[dr-orlovsky]

Sorry, when answering to the issue I was on BTC Prague and answered from the phone with no ability to check.

I recall I had an intention to do such method, but I was not able to find it in the standard library. So I just made one: #221

[nicbus]

I tried to use the new assume_valid method added in #221 to restore the force functionality when accepting a transfer but I cannot do so successfully in rgb-lightning-node, in the context of a LN channel opening.
The issue is that consume_consignment fails when calling create_or_update_state > resolve_height, as the witness anchor has not been broadcast yet, so it can't find it (which is expected).

To me this means that assume_valid might be a way to forcefully validate a consignment, but it's not enough to force the consuming of such a consignment.

[zoedberg]

I've tried the assume_valid method, which fixes the initial issue (i.e. unable to call accept_transfer because I was unable to retrieve a ValidConsignment). I totally agree with Nicola that the method should not be unsafe though. unsafe should not be misused, it should be used only when the compiler needs it because of usage of unsafe Rust. In this case we are not doing anything that's unsafe for the operating system (like for example accessing memory manually), so we should definetely drop it.

After using assume_valid I've investigated the new behavior reported by Nicola and I've noticed an error in the electrum resolver. I've fixed the issue in RGB-WG/rgb#214. After the merge of that PR and the fix and merge of #221 I think we can close this issue.

[dr-orlovsky]

I had a discussion with @nicbus in chat where I explained to him why accepting unverified consignment in funding channel is a security vulnerability and must not be done. The plan that he would discuss that internally with all of you and than close that PR. By all means this method is unsafe in terms of type safety and I am for closing #221 without merging: the method is not needed.

[nicbus]

That discussion led to us checking the behavior of the resolver when accepting a transfer and that's how we discovered that an electrum resolver refactor introduced a bug where it would panic when a transaction was not found, instead of returning it as Offchain. After fixing that, the assume_valid method proved to be correct (with some caveats we're already discussing).

Let's look at an example of what we're doing:

• validating the consignment
• checking that the only failures we get are UnresolvedTransactions for the expected transaction
• once it's passed validation, we call assume_valid to get a ValidConsignment
• we call accept_transfer with that consignment

So the method is needed and does not pose any additional risks compared to other cases where it's not used, not even RGB type safety.

[dr-orlovsky]

My point is that even if you do not misuse the method, others may misuse it. It is wrong to ship a library with a method which can be easily misused creating security vulnerability.

What I would recommend is just do a dedicated resolver wrapping the electrum resolver, name it RlnResolver or something, and let it return the funding transaction to the validator, so you would have ValidConsignment in a natural way, instead of hacking into types with assume_valid footgun.

[nicbus]

My point is that even if you do not misuse the method, others may misuse it. It is wrong to ship a library with a method which can be easily misused creating security vulnerability.

That's not the case. Anyone can modify the code to produce a ValidConsignment anytime. Security doesn't come from the absence of methods to achieve what's needed in some cases.

Moreover this can be mitigated with good documentation. For example, rust-bitcoin has an assume_checked method (https://docs.rs/bitcoin/latest/bitcoin/address/struct.Address.html#method.assume_checked) which if improperly used (to bypass address network validation) could lead to loss of funds. But still, the method could make sense in some special cases, therefore it has been provided and documented as "potentially risky".

What I would recommend is just do a dedicated resolver wrapping the electrum resolver, name it RlnResolver or something, and let it return the funding transaction to the validator, so you would have ValidConsignment in a natural way, instead of hacking into types with assume_valid footgun.

This approach is cumbersome and doesn't add any security, as the reality is that the transaction has not been broadcast and there can be no guarantee that the exact same transaction would be broadcast in the future. Validating the consignment using "fake" data provided by a modified resolver doesn't improve anything.

[fedsten]

@dr-orlovsky did I understood correctly that your main objection here is that some users may misuse the method? If so it doesn't seem to be a strong motivation as good documentation should prevent mistakes from the users. If we were to apply the same logic everywhere we would probably find a lot of other ways in which a user can misuse an RGB library.

[dr-orlovsky]

Well, it is not just a misuse, it is (1) there is no single reason for this method to exist and (2) if it exists it opens a direct security vulnerability.

From what I understood this method is not needed for RGB LN node - and if it is still needed it means the node has a security issue. Under no circumstances an unvalidated consignment should be accepted (the only exception is penetration testing).

[nicbus]

Well, it is not just a misuse, it is (1) there is no single reason for this method to exist and (2) if it exists it opens a direct security vulnerability.

This method is needed, as already explained:

• in LN, when opening a channel, it's essential to create, exchange and verify signatures for the commitment transactions before broadcasting the funding transaction
• in order to create the commitment transactions funding transaction/transfer needs to have been accepted (since the funding UTXO is the input of the commitment TXs)
• there's no other (good) way to accept a transfer for an off-chain transaction

It doesn't open any security vulnerability, as already explained:

• security doesn't come from the absence of this method, as it can always be added by a user (e.g. an rgb-std fork) to carry out an attack
• security needs to be there in spite of this code existing
• this issue and the related PR constitute documentation for a user on how to do this, so it's really easy for anyone
• this can be implemented by anyone even without forking rgb-std using unsafe rust, which would actually be bad
• if done properly, the consignment gets correctly validated, so validation is not skipped but just some specific and expected errors are ignored
• having it, well documented, is better than having users come up with a custom solution, potentially more risky than this method

From what I understood this method is not needed for RGB LN node - and if it is still needed it means the node has a security issue. Under no circumstances an unvalidated consignment should be accepted (the only exception is penetration testing).

It seems like your understanding is that it's not needed because the same effect can be achieved by using a custom resolver that provides fake transaction data. This is not different from having this method and changes nothing on the security side. Using a custom resolver is just another way to force the consignment validation to succeed.

From my understanding of the matter, I conclude that:

• the assume_valid method is the simplest way to achieve what's needed (accept a transfer for a transaction that's not been broadcast yet)
• it has no downsides compared to alternatives
• alternatives are:
  • using a custom resolver to provide fake data and have validation pass
  • modify accept_transfer, re-introducing the previous functionality, but this would mean refactoring the validation as well (it now takes a ValidConsignment)
• I'm against using a custom resolver as it's the most cumbersome approach and requires the user to do a lot of unneeded work, for no gain
• I'm against changing accept_transfer as it means heavily refactoring the code (again) for no real gain

[dr-orlovsky]

It seems like your understanding is that it's not needed because the same effect can be achieved by using a custom resolver that provides fake transaction data.

Yes. That is the only right and secure method of doing it.

This is not different from having this method and changes nothing on the security side.

No it does. Your arguments are superficial:

security doesn't come from the absence of this method, as it can always be added by a user (e.g. an rgb-std fork) to carry out an attack

First, this is not about performing an attack, but about footguns for the users of this library. Second, by the same logic you can add any hacky function to secp245k1 as well, saying otherwise it will be added anyway just by forking the library

this can be implemented by anyone even without forking rgb-std using unsafe rust, which would actually be bad

There is a difference between "footgunny API" which can be simply misused (and docs do not protect from that) and a well-aware unsafe rust with pointers etc.

Sure we can just merge this PR or add any other hacky function you need to the system, but I do believe this is a very bad practice. Thus, if you insist, you can disregard my opinion and merge this PR by yourselves - I will give you the rights to do that (if you do not have that already). But again, my experience of working with Blockstream team on libraries like rust-bitcoin says that better not to do that: never replace a proper API with hacks in the security-critical code just for the sake of simplicity.

[nicbus]

I think it's ok to have the assume_valid method included, restoring the functionality that's been lost from accept_transfer, so I'll take your offer and ask you to allow the merge of #221 (I don't have the permissions). We can have a discussion on how to improve matters separately if needed.

[dr-orlovsky]

restoring the functionality that's been lost from accept_transfer

For the history record, it wasn't "lost functionality", it was "removed bug"

@nicbus I have added you to the maintainers, it should work for you now and you should be able to merge. I pushed commit 63c3f5d so you can merge the PR as is. In any way, please keep my comment there

[nicbus]

@nicbus I have added you to the maintainers, it should work for you now and you should be able to merge. I pushed commit 63c3f5d so you can merge the PR as is. In any way, please keep my comment there

Thanks for that. Anyway I won't be merging that PR, see my comment there.

As a recap, after we had discussions elsewhere, this is no more an issue as the solution provided in #233 is a much better alternative and has already been confirmed to work as intended, so I'm closing this.