search

RGB-WG / rgb-node

RGB node - the official server-side implementation
https://rgb.tech
MIT License
148 stars 43 forks source link

Fix consignment processing #195

Closed zoedberg closed 2 years ago

zoedberg commented 2 years ago

Found some bugs on the consignment processing procedure. I thougt a single PR should ease review, but if needed I'll split this in more PRs.

In Runtime::process_consignment the bundle is stored as a Vec<(NodeId, BTreeSet<u16>)>, but then Collector::process tries to deserialize it into a TransitionBundle like this:

let bundle: TransitionBundle = store
    .retrieve_sten(db::BUNDLES, witness_txid)?
    .ok_or(StashError::BundleAbsent(witness_txid))?;

I've changed how the bundle is stored, assuming we want to store the whole transition bundle and not just concealed transitions.

Then I've changed the way we access the revealed seals, from transition.revealed_seals() to transition.filter_revealed_seals() because the first was always returning an empty list (not sure this is the correct fix or if we should fix the revealed_seals method).

Finally, I added a check (if transition_id.to_vec() == contract_id.to_vec() { continue }) that prevents checking the transition if it refers to the genesis. This check prevents the following failure:

LocalFailure { code: Unknown, message: "transition 50914c89fdc796bcc680b680c4878e39cd3050f5e3a349e63a2d37c46e01dd23 is absent.\nIt may happen due to RGB Node bug, or indicate internal stash inconsistency and compromised stash data storage." }

when looking for the genesis transition. Even here I'm not sure this is the correct fix.

Moreover, after these fixes I'm still struggling to properly receive assets. It seems the ConfidentialSeal never gets "opened" and consequentely saved in the OUTPOINTS table by the receiver, which fails sending the received assets with the following error: UnrelatedInput(OutPoint { txid: 605ee06bd0ae230e8b6d3da92a03448b36f0dbe8f3b991c499955a9a12003932, vout: 1 }).

dr-orlovsky commented 2 years ago

In Runtime::process_consignment the bundle is stored as a Vec<(NodeId, BTreeSet<u16>)>, but then Collector::process tries to deserialize it into a TransitionBundle

That is a clear bug, thank you for catching and fixing.

Then I've changed the way we access the revealed seals, from transition.revealed_seals() to transition.filter_revealed_seals() because the first was always returning an empty list (not sure this is the correct fix or if we should fix the revealed_seals method).

The difference between revealed_seals and filter_revealed_seals methods is that the first one fails if any of the seals are concealed. Since it was followed by unwrap_or_default it resolved into an empty vec. So yes, this is a bug and you correctly fixed it by using right filter_* method there.

Finally, I added a check (if transition_id.to_vec() == contract_id.to_vec() { continue }) that prevents checking the transition if it refers to the genesis.

This is strange, since the way node_ids are collected they should not contain genesis id ever. We need to make sure we do not miss some other bug here. Also pls see my review comment on non-allocating comparison of two ids.

Moreover, after these fixes I'm still struggling to properly receive assets.

From what I heard from Federico it seems you was able to solve that lately?

zoedberg commented 2 years ago

This is strange, since the way node_ids are collected they should not contain genesis id ever. We need to make sure we do not miss some other bug here.

That's good to know, thanks. So the correct fix should not be avoiding processing the genesis ID but instead to make it not appear in the node_ids list at all. I'll give it a look and find where it gets accidentally added.

From what I heard from Federico it seems you was able to solve that lately?

Kinda. I'm using this commit https://github.com/crisdut/rgb-node/commit/494ca080f41b820b8b44561538b241c65a9cf1c3 with a little change for opret. But I'm not sure we really need an accept_transfer API. From what I can see the accept method is implemented very similar to the consume_transfer API (some ops overlapping), so couldn't we just enhance the consume_transfer API so that it takes the revealed seal as an optional parameter and tries to open the concealed seal if that's given?

crisdut commented 2 years ago

Kinda. I'm using this commit crisdut@494ca08 with a little change for opret. But I'm not sure we really need an accept_transfer API. From what I can see the accept method is implemented very similar to the consume_transfer API (some ops overlapping), so couldn't we just enhance the consume_transfer API so that it takes the revealed seal as an optional parameter and tries to open the concealed seal if that's given?

Hi @zoedberg,

I made a branch to extend consume_transfer method with reveal support.

Were you thinking of something like that?

zoedberg commented 2 years ago

Were you thinking of something like that?

@crisdut yes, more or less. The Reveal object you used doesn't include the close method. So either we should directly get an optional seal::Revealed (which includes the close method), or deduce that from somewhere else. Moreover I tried your commit 2ddbf6b (including some little changes for opret and the code of this PR) on rgb-lib tests and it seems that something is missing in this implementation, since I receive an error on spending the received assets. Right now I don't have time to investigate this, sorry, but thanks for your contribution. Maybe here we should first wait @dr-orlovsky to see if he agrees on having a single API with optional reveal and then evenually fix your commit

crisdut commented 2 years ago

@crisdut yes, more or less. The Reveal object you used doesn't include the close method. So either we should directly get an optional seal::Revealed (which includes the close method), or deduce that from somewhere else.

Make sense! I will start tests with opret and update the code.

Moreover I tried your commit 2ddbf6b (including some little changes for opret and the code of this PR) on rgb-lib tests and it seems that something is missing in this implementation, since I receive an error on spending the received assets.

Thanks for feedback, I will check it,

dr-orlovsky commented 2 years ago

Thank you @zoedberg for updating the PR. I propose to continue discussion for the rest of the questions which were not covered in this PR elsewhere - since with merge we can lose the track of them.

dr-orlovsky commented 2 years ago

Linking my answer to @zoedberg question here, since it looks that I put it into a wrong PR: https://github.com/RGB-WG/rgb-node/pull/204#issuecomment-1250233053