Skip to content
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.

Sign up for GitHub

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

feat: ignore the first tx (connect oracle extension tx) #14

Merged
merged 4 commits into from
Nov 29, 2024
Merged

feat: ignore the first tx (connect oracle extension tx) #14

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f306b0
feat: ignore the first tx (connect oracle extension tx)
rustcandy Nov 28, 2024
8e1e8c8
fix: make test pass
rustcandy Nov 28, 2024
dabc56f
fix: change test
rustcandy Nov 28, 2024
1cec91b
fix: check the error message instead of skip the first tx
rustcandy Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions node/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ config:
grpc:
insecure: true
address: "http://localhost:9090"

ignore_connect_vote_extension_tx: false
`

var config nodeconfig.Config
Expand Down Expand Up @@ -57,6 +59,7 @@ config:
max_connections: 10
grpc:
address: http://localhost:9090
ignore_connect_vote_extension_tx: false
hallazzang marked this conversation as resolved.
Show resolved Hide resolved
`
require.Equal(t, strings.TrimLeft(expected, "\n"), string(bz))
}
14 changes: 8 additions & 6 deletions node/remote/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,21 @@ import (

// Details represents a node details for a remote node
type Details struct {
RPC *RPCConfig `yaml:"rpc"`
GRPC *GRPCConfig `yaml:"grpc"`
RPC *RPCConfig `yaml:"rpc"`
GRPC *GRPCConfig `yaml:"grpc"`
IgnoreConnectVoteExtensionTx bool `yaml:"ignore_connect_vote_extension_tx"` // ignore tx[0] for the chains that are using Skip Oracle
}

func NewDetails(rpc *RPCConfig, grpc *GRPCConfig) *Details {
func NewDetails(rpc *RPCConfig, grpc *GRPCConfig, ignoreConnectVoteExtensionTx bool) *Details {
return &Details{
RPC: rpc,
GRPC: grpc,
RPC: rpc,
GRPC: grpc,
IgnoreConnectVoteExtensionTx: ignoreConnectVoteExtensionTx,
}
}

func DefaultDetails() *Details {
return NewDetails(DefaultRPCConfig(), DefaultGrpcConfig())
return NewDetails(DefaultRPCConfig(), DefaultGrpcConfig(), false)
}

// Validate implements node.Details
Expand Down
19 changes: 13 additions & 6 deletions node/remote/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ type Node struct {

computeTxHash types.TxHashCalculator

client *httpclient.HTTP
txServiceClient tx.ServiceClient
client *httpclient.HTTP
txServiceClient tx.ServiceClient
ignoreConnectVoteExtensionTx bool
}

// NewNode allows to build a new Node instance
Expand Down Expand Up @@ -74,8 +75,9 @@ func NewNode(

computeTxHash: txHashCalculator,

client: rpcClient,
txServiceClient: tx.NewServiceClient(grpcConnection),
client: rpcClient,
txServiceClient: tx.NewServiceClient(grpcConnection),
ignoreConnectVoteExtensionTx: cfg.IgnoreConnectVoteExtensionTx,
}, nil
}

Expand Down Expand Up @@ -269,9 +271,14 @@ func (cp *Node) Tx(hash string) (*types.Tx, error) {
}

// Txs implements node.Node

func (cp *Node) Txs(block *tmctypes.ResultBlock) ([]*types.Tx, error) {
txResponses := make([]*types.Tx, len(block.Block.Txs))
for i, tmTx := range block.Block.Txs {
txs := block.Block.Txs
if cp.ignoreConnectVoteExtensionTx && len(block.Block.Txs) > 0 {
txs = block.Block.Txs[1:]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think it's good to check wire type error inside of this logic. I'll add it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by simply checking the wire type error we also prevent any possible other case (e.g. multiple injected transactions)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

}
txResponses := make([]*types.Tx, len(txs))
for i, tmTx := range txs {
txResponse, err := cp.Tx(fmt.Sprintf("%X", cp.computeTxHash(tmTx)))
if err != nil {
return nil, err
Expand Down
Loading