feat: BTC egress witnessing ES #5653
Conversation
marcellorigotti
requested review from
kylezs,
dandanlen and
martin-chainflip
as code owners
marcellorigotti
requested review from
MxmUrw
and removed request for
dandanlen and
martin-chainflip
marcellorigotti
force-pushed
the
feature/pro-2040
branch
2 times, most recently
from
f846c76 to
81c07a1
Compare
marcellorigotti
force-pushed
the
feature/pro-2040
branch
3 times, most recently
from
ed32c18 to
0ae6a73
Compare
marcellorigotti
force-pushed
the
feature/pro-2040
branch
from
0ae6a73 to
9a98dba
Compare
state-chain/chains/src/lib.rs
Outdated
There was a problem hiding this comment.
Also I would hope that with our current approach we don't need these changes as well (referring to the whole chains/src/lib.rs file)? Correct me if I'm wrong though...
There was a problem hiding this comment.
I don't think we can get rid of these changes that easily.. as long as we have this in the ES trait:
type ElectoralUnsynchronisedState: Parameter + Member + MaybeSerializeDeserialize;
Then everything that goes into the BlockProcessor has to to impl Serialize/Deserialize
There was a problem hiding this comment.
If we want, we can do it the same way we did for the Encode/Decode bounds, by adding annotations where we derive Serialize/Deserialize. Here: #5657
There was a problem hiding this comment.
Yeah ofc that's the way to do it, was just having a look at the PR now!
There was a problem hiding this comment.
OTOH it might be fine to simply add these new bounds, the PR above is more of an FYI...
| events: Vec<(BlockNumber, BtcEvent<TransactionConfirmation<Runtime, BitcoinInstance>>)>, | ||
| ) -> Vec<(BlockNumber, BtcEvent<TransactionConfirmation<Runtime, BitcoinInstance>>)> { | ||
| // Map: deposit_witness -> chosen BtcEvent | ||
| // todo! this is annoying, it require us to implement Ord down to the Chain type |
There was a problem hiding this comment.
I think this todo isn't true anymore, right?
| // we don't care about this value for btc (we don't refund out aggkey) | ||
| // the correct value to put here should be the aggKey which signed the tx | ||
| // (either current or previous) I guess we can derive it from | ||
| // VerobseTransaction, if yes how? |
| egress.transaction_ref, | ||
| ) | ||
| .expect("REASON"); | ||
| //todo! handle possible error, expect will cause panic! -> when can this error? |
There was a problem hiding this comment.
This is an interesting case, the egress_success function returns an error if the tx_out_id is unknown to us. This certainly can happen if nodes start voting for wrong txs, but what should we do in that case? Punish them?
Also it feels like we might want to check this already during consensus, such that we don't count votes if they contain invalid txs? But votes are for full blocks, so we would discard the whole vote?
There was a problem hiding this comment.
This should be impossible since in the voter impl we vote only if the hashes match one of our tx, meaning we can only vote for tx_out_id we know.
I'll log in case it errors, like we do for solana
There was a problem hiding this comment.
I think it's worth thinking about the case where the engines don't behave as we expect them to do, to cover all possible cases. Anyways, logging should be fine since nothing of consequence would happen in such an error case.
|
Ok, after thinking a bit about this, I'm fine with introducing these new bounds, since we are gonna effectively require these types to implement Ser/De once we actually start implementing ESs for the other chains. |
Pull Request
Closes: PRO-2040
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
Egress witnessing ES for btc