Add debug method for constructing valid consignments #221
Conversation
There was a problem hiding this comment.
I see the method is documented as "for debug purposes". I don't think this should be a "debug" method, as it's going to be used in "production" code, like accepting the consignment for a funding transaction when opening an RGB LN channel. Having it documented as "debug" makes me think it can be easily dropped in the future without worrying about code that might use it, similar to what happened to the force parameter in accept_transfer. I think this needs to be documented as a method that's intended for special cases and should not be used if you don't know what you're doing, not a debug one.
The method is defined as unsafe, but it doesn't seem unsafe in the rust sense, so I think it should be named unsafe (e.g. assume_valid_unsafe) but not defined as rust unsafe, as that's not the case and it forces users to wrap calls into an unneeded unsafe block.
The comment says this violates type safety. I get the general idea, but what do you mean exactly?
I'm not sure about the custom validation failure. Having a custom warning or info makes sense to me, but a failure feels wrong, as the whole purpose of the method is to render the consignment valid, while a failure indicates validation failed.
|
What you say makes perfect sense if this method is required for normal lightning functioning. But I do not see the scenario whether it is required. Can you please give me more information why a node must accept consignment for lightning channel funding without a validation? The type safety is the fact that given type provides certain guarantees on its data at runtime which are ensured by the compiler at compile-time (here it is the concept of the validation) - meaning that the method is |
A node needs to accept the funding transaction consignment (which allocates RGB assets to the multisig output) before it's broadcast, in order to generate the first pair of commitment transactions (spending the funding and allocating the RGB assets to the local and remote outputs). To be precise, the requirement is not that the consignment is not validated. It just needs to be accepted even if the witness transaction is still missing from the blockchain, as that's expected.
I'm not sure I follow you here. There doesn't seem to be anything that the compiler is checking that would fail if validation is skipped. Instead, it's business logic that's not run when validation is not executed, but this is not at the rust compiler level. To prove that to myself, I dropped the I get your point, you're trying to protect users from unwanted behavior and risks, and I support that, but I don't see the need to use a rust feature in an improper way to drive the point home. Naming and documentation are in my opinion the correct choice here. Moreover, as said above, it's not even the case that this method should do anything "wrong". The need here is to be able to accept a (valid) consignment before the witness transaction has been broadcast. In the context of a funding transaction and a pair of commitment transactions there is no issue, as there's no point in broadcasting a commitment if the funding has never been broadcast itself. On the other hand, broadcasting the funding before constructing the first pair of commitments would be a serious issue. |
Well, we have discussed that: you just need to report the witness transaction as valid: this is the task which must be handled by the lightning node resolver. No need in force-converting anything.
Sorry, now I can't understand what you are saying. Of course if you remove |
For the history record, I am strongly against having and using this method, since it violates the security model. Details: #211 (comment) Strong conceptual NACK on merging this PR
|
For the history record, I am strongly against having and using this method, since it violates the security model. Details: #211 (comment) Strong conceptual NACK on merging this PR |
|
@dr-orlovsky I have pushed a commit on top of yours, to rephrase the docstring and add stronger warnings against the usage of this method. Please let me know if you think this is enough or, if you think even stronger language should be used, what you would add, so I can further improve it. |
| @@ -389,14 +389,14 @@ impl<const TRANSFER: bool> Consignment<TRANSFER> { | |||
| } | |||
| } | |||
|
|
|||
| /// Method to construct valid consignment which can be consumed by a stash | |||
| /// for debug purposes. | |||
| /// Method to forcefully construct a valid consignment, which can be consumed by a stash. | |||
| /// | |||
There was a problem hiding this comment.
Sorry, I strictly disagree with this. It has no - and must not has (!) any other use than the debug purposes, since any other use is literally a security leak. My opinion is so strong, that until the public audit conclusion on this issue I will just fork this repository and will contribute to the fork not having this security issue.
| /// used improperly this could lead to loss of funds. The method must be used only for special | ||
| /// purposes where it's necessary to consume a consignment even though standard rules prevent |
There was a problem hiding this comment.
There are no other "special purposes" than debugging!
|
We had a misunderstanding. I took you giving me permission to merge as an ACK but it's then become clear that that was not the case. It was never my intention to merge this against your will and reasoning. Quite the opposite, the idea was to cooperate to find the best solution. Since then, we had discussions elsewhere and it has all been clarified. This solution is not good for production code, so it's not what we desire either. A better alternative has been found in #233, which makes this PR no more needed, so I'm closing it. |
Closes #211