Node api finality #1897

esuwu · 2025-11-11T09:24:29Z

No description provided.

alexeykiselev · 2025-11-28T07:31:06Z

pkg/wallet/stub.go

-func (s Stub) SignTransactionWith(pk crypto.PublicKey, tx proto.Transaction) error {
-	panic("Stub.SignTransactionWith: Unsupported operation")
+func (s Stub) SignTransactionWith(_ crypto.PublicKey, _ proto.Transaction) error {
+	panic("Stub.SignTransactionWith: Unsopported operation")


Does the word “unsOpported” exist?

alexeykiselev · 2025-11-28T07:36:36Z

pkg/wallet/embedded_wallet.go

+	return crypto.PublicKey{}, ErrPublicKeyNotFound
+}
+
+func (a *EmbeddedWalletImpl) BlsPairByWavesPK(publicKey crypto.PublicKey) (bls.SecretKey, bls.PublicKey, error) {


"Bls" should be "BLS".

alexeykiselev · 2025-11-28T07:41:06Z

pkg/state/commitments.go

+
+// FindEndorserPKsByIndexes returns BLS endorser public keys using
+// commitment indexes stored in FinalizationVoting.EndorserIndexes.
+func (c *commitments) FindEndorserPKsByIndexes(


Why "PKs" and "indexes", not "PK" and "index", the function returns only one public key and accepts only one index.

alexeykiselev · 2025-11-28T07:55:02Z

pkg/state/finalization.go

+	"github.com/wavesplatform/gowaves/pkg/proto"
+)
+
+const finalizationKey = "finalization"


All keys have to be introduced in

gowaves/pkg/state/keys.go

Line 139 in 8b0ac77

commitmentKeyPrefix

alexeykiselev · 2025-11-28T07:56:05Z

pkg/state/finalization.go

+var ErrNoFinalizationHistory = errors.New("no finalization in history")
+
+type finalizationItem struct {
+	Block                proto.BlockHeader `cbor:"0,keyasint,omitempty"`


No need to store block header second time here, can be retrieved by height or blockID.

alexeykiselev · 2025-11-28T08:05:14Z

pkg/state/finalization.go

+func (fr *finalizationRecord) unmarshalBinary(data []byte) error { return cbor.Unmarshal(data, fr) }
+
+type finalizations struct {
+	hs *historyStorage


As we discussed the finalization storage should not be a history storage. In case of using history the rollback will happen automatically, but the idea of finalization records that they are irreversible.

The finalization now contains only one item, I kept the history storage. Moved it to the other PR

alexeykiselev · 2025-11-28T08:07:52Z

pkg/state/appender.go

+		return false, fmt.Errorf("failed to build endorsement message: %w", err)
+	}
+
+	// 2. Восстанавливаем endorser PK по индексам


Oh no! Please use English only.

alexeykiselev · 2025-11-28T08:10:30Z

pkg/state/appender.go

 	return nil
 }

+func (a *txAppender) isLastBlockFinalized(


Strange naming, is it better to name the function "updateFinalization" or "calculateFinalization"?

alexeykiselev · 2025-11-28T08:11:14Z

pkg/state/appender.go

+	if err != nil {
+		return false, err
+	}
+	msg, err := proto.EndorsementMessage(


Why construct an endorsement message here?

Ah, I see, it's for block header validation. I'd rather move this function to proto package, somewhere near block declaration or even made it a part of Block type.

Which file are you suggesting to move it to?

Because this function EndorsementMessage is already in proto

alexeykiselev · 2025-11-28T08:17:12Z

pkg/api/routes.go

Corresponding `pkg/client' parts have to be implemented.

esuwu · 2025-12-03T09:44:11Z

Moved storage to add-network-messages

nickeskov · 2025-12-15T00:39:48Z

pkg/api/app.go

 		return nil, err
 	}

+	settings.GenerationPeriod = cfg.GenerationPeriod


Current argument name collides with the package name. Suggest to rename variable.

pkg/api/app.go

nickeskov · 2025-12-15T00:44:44Z

pkg/api/node_api.go

+	activationHeight, err := a.state.ActivationHeight(int16(settings.DeterministicFinality))
+	if err != nil {
+		return fmt.Errorf("failed to get DeterministicFinality activation height: %w", err)
+	}


What if activation height is unknown at the moment? I think the endpoint returns 500 error in such case.

nickeskov · 2025-12-15T00:45:21Z

pkg/api/node_api.go

+	periodStart, err := state.CurrentGenerationPeriodStart(activationHeight, height,
+		a.app.settings.GenerationPeriod)
+	if err != nil {
+		return err


Add errors context here and below (use fmt.Errorf I mean).

pkg/api/node_api.go

nickeskov · 2025-12-15T01:20:18Z

pkg/api/node_api.go

+type GeneratorInfo struct {
+	Address       string `json:"address"`
+	Balance       uint64 `json:"balance"`
+	TransactionID string `json:"transactionID"`
+}


This struct must be package private.

It's now used in 2 different packages

Yes, but it's an implementation details of our API. It should not be exported to the client packages.

nickeskov · 2025-12-15T01:21:31Z

pkg/client/generators.go

+	if err != nil {
+		return nil, nil, err
+	}
+	var out []api.GeneratorInfo


Better to create new struct for response without using api package.

nickeskov · 2025-12-15T01:23:00Z

pkg/client/generators.go

+	"fmt"
+	"net/http"
+
+	"github.com/wavesplatform/gowaves/pkg/api"


Do not import api package here. api package is our node implementation details, while client is a library.

I don't see a problem in a library using node API structures

nickeskov · 2025-12-15T01:24:19Z

pkg/node/blocks_applier/node_mocks.go

No changes are necessary

nickeskov · 2025-12-15T01:27:16Z

pkg/wallet/stub.go

This file can be completely removed. At least Deprecated.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 11 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T23:05:04Z

pkg/client/transactions.go

 }
+
+type SignCommitRequest struct {
+	Sender                string  `json:"sender"`


The SignCommitRequest struct is defined in both pkg/client/transactions.go and pkg/api/node_api.go with different fields. The client version lacks the ChainID field that is present in the API version. This inconsistency could lead to issues when the client sends requests to the API endpoint, as the client won't be able to specify ChainID even if needed. Consider consolidating these struct definitions or ensuring the client struct includes all fields that the API can accept.

Suggested change

Sender string `json:"sender"`

Sender string `json:"sender"`

ChainID *byte `json:"chainId,omitempty"`

Copilot · 2025-12-17T23:05:05Z

pkg/api/node_api.go

+	ChainID               *byte   `json:"chainId,omitempty"`
+}


The ChainID field in SignCommitRequest is accepted but never used in the TransactionsSignCommit function. This field should either be removed from the struct if it's not needed, or the value should be used appropriately when creating or signing the transaction.

Copilot · 2025-12-17T23:05:05Z

pkg/wallet/embedded_wallet.go

+			secretKeyBls, genErr := bls.GenerateSecretKey(s)
+			if genErr != nil {
+				return bls.SecretKey{}, bls.PublicKey{}, genErr
+			}
+			publicKeyBls, retrieveErr := secretKeyBls.PublicKey()
+			if retrieveErr != nil {
+				return bls.SecretKey{}, bls.PublicKey{}, retrieveErr


Variable naming is inconsistent within this function. The function uses 'publicKeyRetrieved' for the generated public key but then uses different error variable names (genErr, retrieveErr) which breaks the naming pattern. Consider using more consistent naming like 'err' for all error variables or using a consistent pattern throughout.

Suggested change

secretKeyBls, genErr := bls.GenerateSecretKey(s)

if genErr != nil {

return bls.SecretKey{}, bls.PublicKey{}, genErr

}

publicKeyBls, retrieveErr := secretKeyBls.PublicKey()

if retrieveErr != nil {

return bls.SecretKey{}, bls.PublicKey{}, retrieveErr

secretKeyBls, err := bls.GenerateSecretKey(s)

if err != nil {

return bls.SecretKey{}, bls.PublicKey{}, err

}

publicKeyBls, err := secretKeyBls.PublicKey()

if err != nil {

return bls.SecretKey{}, bls.PublicKey{}, err

Bad AI, your suggestion will shadow errors, which is a worse pattern

Copilot · 2025-12-17T23:05:06Z

pkg/api/node_api.go

+	var generatorsInfo []GeneratorInfo
+
+	generatorAddresses, err := a.state.CommittedGenerators(periodStart)
+	if err != nil {
+		return err
+	}
+	for _, generatorAddress := range generatorAddresses {
+		endorserRecipient := proto.NewRecipientFromAddress(generatorAddress)
+		balance, pullErr := a.state.GeneratingBalance(endorserRecipient, height)
+		if pullErr != nil {
+			return pullErr
+		}
+		generatorsInfo = append(generatorsInfo, GeneratorInfo{
+			Address:       generatorAddress.String(),
+			Balance:       balance,
+			TransactionID: "", // TODO should be somehow found.
+		})
+	}


The generatorsInfo slice is appended to without pre-allocation. Since the number of generators is known from len(generatorAddresses), consider pre-allocating the slice with make([]GeneratorInfo, 0, len(generatorAddresses)) to avoid potential reallocation overhead during iteration.

Copilot · 2025-12-17T23:05:06Z

pkg/wallet/embedded_wallet.go

+func (a *EmbeddedWalletImpl) FindPublicKeyByAddress(address proto.WavesAddress,
+	scheme proto.Scheme) (crypto.PublicKey, error) {
+	seeds := a.seeder.AccountSeeds()
+	for _, s := range seeds {
+		_, public, err := crypto.GenerateKeyPair(s)
+		if err != nil {
+			return crypto.PublicKey{}, err
+		}
+		retrievedAddress, err := proto.NewAddressFromPublicKey(scheme, public)
+		if err != nil {
+			return crypto.PublicKey{}, err
+		}
+		if retrievedAddress == address {
+			return public, nil
+		}
+	}
+	return crypto.PublicKey{}, ErrPublicKeyNotFound
+}


The new FindPublicKeyByAddress method lacks test coverage. Since the existing SignTransactionWith method has tests in embedded_wallet_test.go, consider adding test cases for this new method to ensure it correctly finds public keys by address and handles error cases like when the address is not found.

Copilot · 2025-12-17T23:05:06Z

pkg/client/blocks.go

+func (a *Blocks) HeightFinalized(ctx context.Context) (uint64, *Response, error) {
+	url, err := joinUrl(a.options.BaseUrl, "/blocks/height/finalized")
+	if err != nil {
+		return 0, nil, err
+	}
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url.String(), nil)
+	if err != nil {
+		return 0, nil, err
+	}
+
+	var out struct {
+		Height uint64 `json:"height"`
+	}
+
+	resp, err := doHTTP(ctx, a.options, req, &out)
+	if err != nil {
+		return 0, resp, err
+	}
+
+	return out.Height, resp, nil
+}
+
+func (a *Blocks) BlockFinalized(ctx context.Context) (*proto.BlockHeader, *Response, error) {
+	url, err := joinUrl(a.options.BaseUrl, "/blocks/headers/finalized")
+	if err != nil {
+		return nil, nil, err
+	}
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url.String(), nil)
+	if err != nil {
+		return nil, nil, err
+	}
+	var out proto.BlockHeader
+	resp, err := doHTTP(ctx, a.options, req, &out)
+	if err != nil {
+		return nil, resp, err
+	}
+	return &out, resp, nil
+}


The new HeightFinalized and BlockFinalized methods lack test coverage. Since other methods in blocks_test.go have comprehensive tests, consider adding test cases for these new finalized block endpoints to ensure they correctly retrieve finalized data and handle error cases.

Copilot · 2025-12-17T23:05:07Z

pkg/client/generators.go

+package client
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+
+	"github.com/wavesplatform/gowaves/pkg/api"
+)
+
+// Generators is a client wrapper for generator-related API endpoints.
+type Generators struct {
+	options Options
+}
+
+func NewGenerators(options Options) *Generators {
+	return &Generators{
+		options: options,
+	}
+}
+
+// GeneratorsAtResponse is the expected structure returned by /generators/at/{height}.
+type GeneratorsAtResponse struct {
+	Height     uint64   `json:"height"`
+	Generators []string `json:"generators"`
+}
+
+// CommitmentGeneratorsAt returns the list of committed generators for the given height.
+func (a *Generators) CommitmentGeneratorsAt(ctx context.Context,
+	height uint64) ([]api.GeneratorInfo, *Response, error) {
+	url, err := joinUrl(a.options.BaseUrl, fmt.Sprintf("/generators/at/%d", height))
+	if err != nil {
+		return nil, nil, err
+	}
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url.String(), nil)
+	if err != nil {
+		return nil, nil, err
+	}
+	var out []api.GeneratorInfo
+	resp, err := doHTTP(ctx, a.options, req, &out)
+	if err != nil {
+		return nil, resp, err
+	}
+
+	return out, resp, nil
+}


The new Generators client wrapper lacks test coverage. Since other client files in this directory have corresponding test files, consider creating generators_test.go with tests for the CommitmentGeneratorsAt method.

Copilot · 2025-12-17T23:05:07Z

pkg/wallet/embedded_wallet.go

+func (a *EmbeddedWalletImpl) BLSPairByWavesPK(publicKey crypto.PublicKey) (bls.SecretKey, bls.PublicKey, error) {
+	seeds := a.seeder.AccountSeeds()
+	for _, s := range seeds {
+		_, publicKeyRetrieved, err := crypto.GenerateKeyPair(s)
+		if err != nil {
+			return bls.SecretKey{}, bls.PublicKey{}, err
+		}
+		if publicKeyRetrieved == publicKey {
+			secretKeyBls, genErr := bls.GenerateSecretKey(s)
+			if genErr != nil {
+				return bls.SecretKey{}, bls.PublicKey{}, genErr
+			}
+			publicKeyBls, retrieveErr := secretKeyBls.PublicKey()
+			if retrieveErr != nil {
+				return bls.SecretKey{}, bls.PublicKey{}, retrieveErr
+			}
+			return secretKeyBls, publicKeyBls, nil
+		}
+	}
+	return bls.SecretKey{}, bls.PublicKey{}, ErrPublicKeyNotFound
+}


The new BLSPairByWavesPK method lacks test coverage. Since the existing SignTransactionWith method has tests in embedded_wallet_test.go, consider adding test cases for this new method to verify it correctly generates BLS key pairs from Waves public keys and handles error cases appropriately.

Copilot · 2025-12-17T23:05:07Z

pkg/client/generators.go

+// GeneratorsAtResponse is the expected structure returned by /generators/at/{height}.
+type GeneratorsAtResponse struct {
+	Height     uint64   `json:"height"`
+	Generators []string `json:"generators"`
+}
+


The GeneratorsAtResponse struct is defined but never used. The CommitmentGeneratorsAt method returns []api.GeneratorInfo instead. Consider removing this unused struct or using it as the actual return type if that was the original intent.

Suggested change

// GeneratorsAtResponse is the expected structure returned by /generators/at/{height}.

type GeneratorsAtResponse struct {

Height uint64 `json:"height"`

Generators []string `json:"generators"`

}

Copilot · 2025-12-17T23:05:07Z

pkg/api/node_api.go

+		generatorsInfo = append(generatorsInfo, GeneratorInfo{
+			Address:       generatorAddress.String(),
+			Balance:       balance,
+			TransactionID: "", // TODO should be somehow found.


The TransactionID field is always set to an empty string with a TODO comment indicating it should be found somehow. This incomplete implementation means the API returns incomplete data to clients. Consider either implementing the lookup for TransactionID or documenting why it's not available and potentially making the field optional or removing it if it cannot be populated.

Suggested change

TransactionID: "", // TODO should be somehow found.

TransactionID: "", // TransactionID is currently not available and remains empty for backward compatibility.

nickeskov · 2025-12-23T01:03:16Z

pkg/api/errors.go

+		authError        *AuthError
+		unknownError     *apiErrs.UnknownError
+		apiError         apiErrs.ApiError
+		unavailableError *apiErrs.UnavailableError


New error should implement ApiError interface.

They already do

nickeskov · 2025-12-23T02:40:00Z

pkg/api/errors/consts.go

 // API Auth
 const (
-	ApiKeyNotValidErrorID        ApiAuthErrorID = 2
+	APIKeyNotValidErrorID        ApiAuthErrorID = 4


These constants IDs should not be changed.

Why is that?

nickeskov · 2025-12-23T02:40:59Z

pkg/api/node_api.go

+type GeneratorInfo struct {
+	Address       string `json:"address"`
+	Balance       uint64 `json:"balance"`
+	TransactionID string `json:"transactionID"`
+}


Yes, but it's an implementation details of our API. It should not be exported to the client packages.

nickeskov · 2025-12-23T02:43:04Z

pkg/api/routes.go

+
+			rAuth := r.With(checkAuthMiddleware)
+			rAuth.Post("/sign", wrapper(a.TransactionsSignCommit))


we need to discuss it with the team.

Agreed

esuwu added 6 commits November 11, 2025 10:23

Drafted endpoints for API

d5bcb7d

Updated protobuf structures

4be1b1e

Merge branch 'add-network-messages' into node-api-finality

52af168

Added finalization storage

d033dc9

Fixed errors

402ef6f

Fixed old code

1caf2a1

esuwu requested a review from alexeykiselev November 24, 2025 09:13

esuwu added 5 commits November 24, 2025 11:27

SignCommitGeneration draft

82a7a2e

Merged from finality network messages

aac66f8

Finished signCommitToGeneration

350e93d

Merged from add network messages

9b96f0f

Added validation of finalization

20bf1d8

alexeykiselev requested changes Nov 28, 2025

View reviewed changes

esuwu added 3 commits December 3, 2025 09:50

Added finalization validation

282bf9f

Added a finalization processor for tx appender

cd82ba3

Merged from add-network-messages

51f714d

esuwu added 5 commits December 5, 2025 14:24

Merged from add-network-messages

44ad778

Merged again

40c3d02

Added clients methods

5dce8da

Added http client itests

b77abc1

Fixed an error

6e83e89

nickeskov requested review from Copilot and nickeskov December 15, 2025 00:56

Copilot started reviewing on behalf of nickeskov December 15, 2025 00:57 View session

nickeskov reviewed Dec 15, 2025

View reviewed changes

Copilot AI reviewed Dec 15, 2025

View reviewed changes

esuwu added 2 commits December 17, 2025 14:28

Updated protobuf version

2bfd983

Merge branch 'add-network-messages' into node-api-finality

d17d321

nickeskov requested a review from Copilot December 17, 2025 22:59

Copilot started reviewing on behalf of nickeskov December 17, 2025 22:59 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

esuwu added 3 commits December 18, 2025 10:12

Merge branch 'add-network-messages' into node-api-finality

a504094

Implemented nickeskov's suggestions

1258e13

Implemented some AI suggestions

8b80e20

nickeskov reviewed Dec 23, 2025

View reviewed changes

Made the tx signing handle all tx types

57f2cf6

	Sender string `json:"sender"`
	Sender string `json:"sender"`
	ChainID *byte `json:"chainId,omitempty"`

	TransactionID: "", // TODO should be somehow found.
	TransactionID: "", // TransactionID is currently not available and remains empty for backward compatibility.


		rAuth := r.With(checkAuthMiddleware)
		rAuth.Post("/sign", wrapper(a.TransactionsSignCommit))

Node api finality #1897

Are you sure you want to change the base?

Node api finality #1897

Uh oh!

Conversation

esuwu commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esuwu commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment