feat: ignore the first tx (connect oracle extension tx) #14
Conversation
|
Just to provide others with more information about this: Skip's oracle rejects a proposal when there isn't an injected tx(which essentially is the current block's extended commit info): ref. So if we have a Connect-enabled chain then we can assure that every blocks in the chain will have this injected transaction as their first transaction in the list. |
node/remote/node.go
Outdated
| for i, tmTx := range block.Block.Txs { | ||
| txs := block.Block.Txs | ||
| if cp.ignoreConnectVoteExtensionTx && len(block.Block.Txs) > 0 { | ||
| txs = block.Block.Txs[1:] |
There was a problem hiding this comment.
One issue that I have with this implementation is: what if we don't have vote extensions in all blocks?
I think it would be better to instead skip transactions when trying to de-serialize them rather than here. So, inside the Tx method rather than Txs: if ignoreConnectVoteExtensionTx is true and we get a wire type error, then we ignore that transaction. Otherwise we proceed as usual.
What do you think @hallazzang @rustcandy?
There was a problem hiding this comment.
It looks like always at the first place of txs according to the code @hallazzang found above. Do you think there's still possibility it's missing somewhere?
There was a problem hiding this comment.
Yes, for example in a chain that supports VoteExtensions but is not able to fetch the prices. I am not sure whether the transaction in that case would be there or not. For example a single-validator chain that only enables connect at height 20: for all previous blocks there shouldn't be that kind of transaction. Or am I wrong?
There was a problem hiding this comment.
There's error on My localnet: is this the case?
3:48PM ERR oracle returned nil prices for vote extension; returning empty vote extension height=2601 module=server
This case, there's still tx[0] exist.
There was a problem hiding this comment.
But I think it's good to check wire type error inside of this logic. I'll add it
There was a problem hiding this comment.
I think by simply checking the wire type error we also prevent any possible other case (e.g. multiple injected transactions)
There was a problem hiding this comment.
@RiccardoM Actually that was a valid concern. There is a configuration called abci.vote_extensions_enable_height inside ConsensusParams so I think this is true:
For example a single-validator chain that only enables connect at height 20: for all previous blocks there shouldn't be that kind of transaction
|
@RiccardoM should I check just |
Description
@hallazzang found that the skip oracle injects their oracle data at tx[0] always:
https://github.com/skip-mev/connect/blob/7a115b15fd218eae84e42195941c5a3b0761d2eb/abci/proposals/proposals.go#L222
We need to skip this tx for milkyway chain
Checklist
Files changedin the Github PR explorer.