-
-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor types for ordering global state in contract history data #169
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, see my comments.
src/contract/contract.rs
Outdated
} | ||
|
||
impl WitnessOrd { | ||
pub fn from_electrum_height(height: u32) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we intend to support other explorers, like esplora, I think it's interesting to change this name.
pub fn from_electrum_height(height: u32) -> Self { | |
pub fn from_block_height(height: u32) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's not block height, it is just an Electrum API which reports 0 for mempool (Bitcoin Core doesn't do that; esplora inherits electrum API). The name MUST indicate that this is something very custom and not a part of anything standard (like block height)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be something like from_explorer_height
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/contract/contract.rs
Outdated
txid, | ||
} | ||
} | ||
pub fn with_electrum_height(height: u32, txid: Txid) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we intend to support other explorers, like esplora, I think it's interesting to change this name.
pub fn with_electrum_height(height: u32, txid: Txid) -> Self { | |
pub fn with_block_height(height: u32, txid: Txid) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything works in my test suite.
This is the most minimalistic solution to the problem at the consensus level.
The way it will work is the following: we now guarantee that all 0-heights are considered as the latest updates to the state.
During the validation, all mempool-based global state is ignored (it is not accessible to the VM the same way "mined" global state is; instead VM uses special procedure to access the global state from the currently validated terminal state transition).
For the lightning channel updates (or in any state channels), we do not send or validate consignments - instead, each party constructs state transitions and keeps it OUTSIDE of RGB stash (in LNP node) - alongside history of signatures for bitcoin tx. Thus, these state transitions are never validated with a consensus code/RGB Core.
So I think we should not go further and this is sufficient solution