Conversation
| @@ -0,0 +1,11 @@ | |||
| package test | |||
There was a problem hiding this comment.
Now that you have brought back the internal directory, I think it'd make sense to delete test/factory/tx.go in favor of this file (they are duplicated AFAICT).
There was a problem hiding this comment.
Yeah, they're duplicates. Have removed test/factory/tx.go
types/evidence.go
Outdated
| idx, val := valSet.GetByAddress(vote1.ValidatorAddress) | ||
| if idx == -1 { | ||
| return nil | ||
| return nil, errors.New("validator not in validator set") |
There was a problem hiding this comment.
should we put the name of the validator in the error message?
There was a problem hiding this comment.
Yeah that's a good idea. 1 sec
internal/test/commit.go
Outdated
| "github.com/tendermint/tendermint/types" | ||
| ) | ||
|
|
||
| func MakeCommit(ctx context.Context, blockID types.BlockID, height int64, round int32, voteSet *types.VoteSet, validators []types.PrivValidator, now time.Time) (*types.Commit, error) { |
There was a problem hiding this comment.
This one looks the same to me as function MakeCommit in types/test_util.go, can you double-check?
There was a problem hiding this comment.
Yeah, there's a lot of deduplication in the testing suite that we can do. I'm not sure if we should do it all in this PR though
There was a problem hiding this comment.
Yeah, up to you. In different circumstances I'd insist, but we're in the critical path here.
| "github.com/tendermint/tendermint/types" | ||
| ) | ||
|
|
||
| func MakeVote( |
There was a problem hiding this comment.
There seems to be some overlapping of this function with the ones defined in:
types/test_util.gotypes/evidence_test.goevidence/verify_test.go
It would be awesome if we took the opportunity to consolidate all three.
There was a problem hiding this comment.
Just as my comment above: up to you if you want to do it as part of this PR, or later
|
|
||
| [node.seed01] | ||
| mode = "seed" | ||
| seeds = ["seed02"] |
There was a problem hiding this comment.
Why are we removing seed02? (not that I'm against, just trying to understand the rationale)
There was a problem hiding this comment.
It was moved in the original PR. I think the rationale was to reduce the size of ci.toml. Having a secondary seed doesn't seem to provide any additional value
| PrepareProposalDelay: manifest.PrepareProposalDelay, | ||
| ProcessProposalDelay: manifest.ProcessProposalDelay, | ||
| CheckTxDelay: manifest.CheckTxDelay, |
There was a problem hiding this comment.
Same comment, please double-check this hunk
| @@ -0,0 +1,320 @@ | |||
| package main | |||
There was a problem hiding this comment.
I'm not reviewing this file line-by-line as I assume it comes as-is from some cherry-pick.
Let me know if that's not the case, and I'll go through it more thoroughly.
| "prepare_proposal_delay": node.Testnet.PrepareProposalDelay, | ||
| "process_proposal_delay": node.Testnet.ProcessProposalDelay, | ||
| "check_tx_delay": node.Testnet.CheckTxDelay, |
There was a problem hiding this comment.
Please doublecheck these.
| "persist_interval": node.PersistInterval, | ||
| "snapshot_interval": node.SnapshotInterval, | ||
| "retain_blocks": node.RetainBlocks, | ||
| "key_type": node.PrivvalKey.Type(), |
There was a problem hiding this comment.
Do we need to add some evidence-related entry here?
Closes: #9237