Skip to content

replacing abci protobuf types with plain golang types (part 1)#2935

Merged
pompon0 merged 36 commits intomainfrom
gprusak-execute2b
Feb 23, 2026
Merged

replacing abci protobuf types with plain golang types (part 1)#2935
pompon0 merged 36 commits intomainfrom
gprusak-execute2b

Conversation

@pompon0
Copy link
Contributor

@pompon0 pompon0 commented Feb 19, 2026

I'm working on reducing the API surface between consensus and Application for autobahn migration. This PR replaces protobuf types with plain golang types to reduce compatibility requirements. Will continue in the next pr.

@github-actions
Copy link

github-actions bot commented Feb 19, 2026

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedFeb 23, 2026, 4:36 PM

@codecov
Copy link

codecov bot commented Feb 19, 2026

Codecov Report

❌ Patch coverage is 11.82796% with 164 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.75%. Comparing base (f23d05a) to head (77ccfc9).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
sei-tendermint/abci/types/plain_types.go 11.82% 159 Missing and 5 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2935       +/-   ##
===========================================
+ Coverage   57.83%   60.75%    +2.92%     
===========================================
  Files        2110       29     -2081     
  Lines      174010     3585   -170425     
===========================================
- Hits       100633     2178    -98455     
+ Misses      64425     1217    -63208     
+ Partials     8952      190     -8762     
Flag Coverage Δ
sei-chain 59.62% <11.82%> (+1.81%) ⬆️
sei-db 68.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-tendermint/abci/types/application.go 34.88% <ø> (+1.55%) ⬆️
sei-tendermint/abci/types/plain_types.go 11.82% <11.82%> (ø)

... and 2081 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pompon0 pompon0 changed the title Gprusak execute2b replacing abci protobuf types with plain golang types (part 1) Feb 20, 2026
@pompon0 pompon0 requested review from masih and wen-coding February 20, 2026 08:40
@pompon0 pompon0 marked this pull request as ready for review February 20, 2026 08:42
Copy link
Collaborator

@masih masih left a comment

Choose a reason for hiding this comment

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

One potential blocker on backward compatibility. Left a bunch of suggestions worth taking a look.

Loving the 3X code reduction diff 🙌

)

//go:generate ../../scripts/mockery_generate.sh Client

Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

}

// RequestProcessProposal is the plain Go replacement for the legacy protobuf message.
type RequestProcessProposal struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can extract the common fileds across this type and RequestFinalizeBlock into an unexported type, e.g. blockRequestBase to reduce boiler plating. Something like this:

type blockRequestBase struct {
    Txs                   [][]byte
    ByzantineValidators   []Misbehavior
    Hash                  []byte
    Height                int64
    Time                  time.Time
    NextValidatorsHash    []byte
    ProposerAddress       []byte
    AppHash               []byte
    ValidatorsHash        []byte
    ConsensusHash         []byte
    DataHash              []byte
    EvidenceHash          []byte
    LastBlockHash         []byte
    LastBlockPartSetTotal int64
    LastBlockPartSetHash  []byte
    LastCommitHash        []byte
    LastResultsHash       []byte
}

Then these types would become:

type RequestProcessProposal struct {
    blockRequestBase
    ProposedLastCommit CommitInfo
}

type RequestFinalizeBlock struct {
    blockRequestBase
    DecidedLastCommit CommitInfo
}

Which makes semantic differences obvious/easy to see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the plan is to use block header, because that's where all these fields come from, and it doesn't seem feasible to make block header an implementation detail any time soon (there are APIs and storage depending on it). Refactoring this will be the next step.

LastBlockPartSetHash []byte
LastCommitHash []byte
LastResultsHash []byte
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking out loud: Do we need validator for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wdym?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean: do any of the types here need validation? We might have validation already somewhere. Checking that we do validate fields if they come off the wire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are passed from consensus to application. Application does not interpret most of these, doesn't even access many of those, but still persists it at places. Presumably so that the header hash can be computed by light clients or sth.

LastResultsHash []byte
}

func (m *RequestProcessProposal) GetTxs() [][]byte {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Beyond the scope of this PR:

Do we still need these getters? Aside from >400 lies of bilerplate they add, these have a hidden cost in that they encourage pointer passing. They make sense for a one-size-cut-all Proto generator but for hand-rolled types we could be more concise/robust.

I understand that removing these needs a careful review to make sure we are not missing any nil checks. But we all will be better off for it: in that it forces us to validate earlier, and interrupt the execution flow when it makes no sense to proceed. e.g. when *RequestProcessProposal is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the getters either, although they do not block my goal of reducing the api surface. +1 for getting rid of them though

Copy link
Collaborator

@masih masih left a comment

Choose a reason for hiding this comment

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

The one blocker comment discussed/resolved. Approving

@pompon0 pompon0 enabled auto-merge (squash) February 23, 2026 16:52
@pompon0 pompon0 merged commit 462deb2 into main Feb 23, 2026
35 checks passed
@pompon0 pompon0 deleted the gprusak-execute2b branch February 23, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants