mutable consensus parameters #1213

jchappelow · 2025-01-10T19:27:56Z

Based on top of #1222

This aims to make consensus parameters mutable and add the update and proposal code to support changing them. The RPCs and CLI commands may be removed for product, but I needed them to test the internals e2e.

This needs more work for sure, but I'm putting it up as draft anyway since I'm not progressing quickly today.

Part of this branch includes changes related to key type, signers, authenticators, etc.

Usage:
  kwild consensus [command]

Available Commands:
  approve        Approve a consensus update proposal.
  list-proposals List all the pending consensus update proposals.
  params         Show active consensus parameters.
  propose        Submit a migration proposal.
  update-status  Pending consensus update proposal status.

$  ./kwild consensus params
Network Parameters:
	Leader: 0226b3ff29216dac187cea393f8af685ad419ac9644e55dce83d145c8b1af213bd [secp256k1]
	DB Owner: bababa
	Max Block Size: 6291456
	Join Expiry: 14400
	Vote Expiry: 108000
	Disabled Gas Costs: true
	Max Votes Per Tx: 200
	Migration Status: NoActiveMigration

$  ./kwild consensus propose -d "testchange" -u '{"max_block_size": 99999}'
Please review the following proposal:

Description: testchange

Updates: {"max_block_size":99999}

Do you want to submit this proposal? (y/n): y
TxHash: 6f56d7b67f8a4261e08a47b7a60092c90f47ce8665b4b6e810af120f1d284dac

$  ./kwild consensus params
Network Parameters:
	Leader: 0226b3ff29216dac187cea393f8af685ad419ac9644e55dce83d145c8b1af213bd [secp256k1]
	DB Owner: bababa
	Max Block Size: 99999
	Join Expiry: 14400
	Vote Expiry: 108000
	Disabled Gas Costs: true
	Max Votes Per Tx: 200
	Migration Status: NoActiveMigration

Also tested with db owner. Just with a single leader and no validators, more to check...

We'll probably need update hooks as simple changing the network parameters struct is unlikely to be sufficient for every change. e.g. I imagine db owner changing will involve DB query action. We probably also do not want to permit migration status to be arbitrarily changed since we have other mechanisms for that.

jchappelow

Notes and comments mainly for @charithabandi and @brennanjl , but interested in thoughts from all.

jchappelow · 2025-01-13T16:16:36Z

app/node/build.go

@@ -139,12 +143,9 @@ func buildServer(ctx context.Context, d *coreDependencies) *server {
 		jsonRPCAdminServer.RegisterSvc(jsonAdminSvc)
 		jsonRPCAdminServer.RegisterSvc(jsonRPCTxSvc)
 		jsonRPCAdminServer.RegisterSvc(&funcsvc.Service{})
+		jsonRPCAdminServer.RegisterSvc(jsonChainSvc)


NOTE: @charithabandi user and admin servers now register all the "chain" service methods.
The client types now also include chain service client method.

jchappelow · 2025-01-13T16:17:46Z

app/params/cmd.go

+var consensusCmd = &cobra.Command{
+	Use:   "consensus",
+	Short: "Functions for dealing with consensus update proposals.",
+	Long:  "The `consensus` command provides functions for dealing with consensus update proposals.",
+}
+
+func NewConsensusCmd() *cobra.Command {
+	consensusCmd.AddCommand(
+		proposeUpdatesCmd(),
+		listUpdateProposalsCmd(),
+		approveUpdateProposalCmd(),
+		showUpdateProposalCmd(),
+		showConsensusParams(),


All of these kwild consensus commands are mainly for my testing. We can hide or change entirely for beta release.

These are pretty useful, lets keep them

jchappelow · 2025-01-13T16:19:24Z

cmd/kwil-cli/cmds/account/account.go

+		addr, err := auth.EthSecp256k1Authenticator{}.Identifier(signer.CompactID())
+		if err != nil {
+			return display.PrintErr(cmd, err)
+		}
+		return display.PrintCmd(cmd, display.RespString(addr))


This is the change to kwil-cli account id to print the checksumed and 0x prefixed adddress.

jchappelow · 2025-01-13T16:20:29Z

cmd/kwil-cli/cmds/account/balance.go

@@ -35,7 +35,7 @@ func balanceCmd() *cobra.Command {
 						return display.PrintErr(cmd, errors.New("no account ID provided and no signer set"))
 					}

-					ident, err := auth.Secp25k1Authenticator{}.Identifier(cl.Signer().Identity())
+					ident, err := auth.Secp25k1Authenticator{}.Identifier(cl.Signer().CompactID())


For account balance where we request balance based on account, this may change depending on how we decide to handle the account store. Maybe it becomes more like current voting/validator store where ID and key type are required, but with ID and auth type since this is conceptually related to engine @caller.

I am gonna make the accounts changes today, will probably take the similar approach as Votestore.

jchappelow · 2025-01-13T16:22:09Z

common/context.go

+	NetworkParameters *types.NetworkParameters
+
+	NetworkUpdates types.ParamUpdates


The overall design is that during block execution, resolutions that when update resolutions are resolved, they modify this NetworkUpdates field of ChainContext, and on block commit, they are merged into the active *NetworkParameters.

jchappelow · 2025-01-13T16:41:13Z

node/block_processor/processor.go

+	inMigration := bp.chainCtx.NetworkParameters.MigrationStatus == ktypes.MigrationInProgress
+	haltNetwork := bp.chainCtx.NetworkParameters.MigrationStatus == ktypes.MigrationCompleted


This is not new code, but it is notable in that it modifies the MigrationStatus during block execution rather than after in commit. I think this is known, but wanted to call it out in case it should be handled in the ParamUpdates instead.

node/consensus/engine.go

jchappelow · 2025-01-13T16:47:20Z

node/consensus/updates.go

+var ParamUpdatesResolution = resolutions.ResolutionConfig{
+	ConfirmationThreshold: big.NewRat(2, 3),
+	ExpirationPeriod:      7 * 24 * 60 * 10, // assumes 10 minute blocks
+	ResolveFunc: func(ctx context.Context, app *common.App, resolution *resolutions.Resolution, block *common.BlockContext) error {
+		// a resolution with an invalid body should be rejected before this
+		var pud ParamUpdatesDeclaration
+		err := pud.UnmarshalBinary(resolution.Body)
+		if err != nil {
+			return err
+		}
+
+		app.Service.Logger.Info("Applying param updates", "description", pud.Description, "paramUpdates", pud.ParamUpdates)
+
+		// block.ChainContext.NetworkUpdates <= pud.ParamUpdates
+		if block.ChainContext.NetworkUpdates == nil {
+			block.ChainContext.NetworkUpdates = make(types.ParamUpdates, len(pud.ParamUpdates))
+		}
+		block.ChainContext.NetworkUpdates.Merge(pud.ParamUpdates)
+		return nil
+	},
+}


A couple important decisions / questions here

What is the "right" way presently to handle error returns from as ResolveFunc? Should it never return errors? Does the caller just log and not crash the node? Can/should the ResolutionConfig include a BodyValidation function? In the caller we have if the resolveFunc fails, we should still continue on, since it simply means documented, but I want to confirm and consider documenting this in the types.

While this code does the merge of updates into the active parameters, it's really not so simple for every parameter. e.g. changing DB owner needs some other actions almost certainly. MigrationStatus, while mutable and grouped with the other mutable params, should not be changed via this mechanism, right? Leader can be changed in this updates mechanism, but like DB owner, there would have to be other actions or at least the change detected by CE. There can be higher level code that detects changes, or there can be some hooks that get called here, although I'm haven't explored how to define and inject them.

As Resolutions can be defined by the clients and can have impact on the consensus, we didn't want these to stop the node unless there is some non-determinism. So this approach of not returning any errors back to TxApp.

Huh, I think we can have all required modules subscribe to the consensus parameters and let them know the final params at the end of every commit and the modules can take appropriate actions based on the updated state.

2. Huh, I think we can have all required modules subscribe to the consensus parameters and let them know the final params at the end of every commit and the modules can take appropriate actions based on the updated state.

This feels right to me too. The full params can go to subscribers and they can decide what to do with it.

Making this comment thread into an issue so we can get this PR merged.

jchappelow · 2025-01-13T16:48:48Z

node/meta/meta.go

+	initParamsTable = `CREATE TABLE IF NOT EXISTS ` + chainSchemaName + `.params (
+		params BYTEA
+	);` // no primary key, only one row


We can re-fancy this if needed. It just very straightforward to have it store the binary marshaled params. It is also extremely cheap just to write them at the end of each block and not try to compute "diffs", which is error prone and requires maintenance.

jchappelow · 2025-01-13T16:49:45Z

node/node.go

+			TxAnnouncer: func(ctx context.Context, txHash types.Hash, rawTx []byte) {
+				n.announceTx(ctx, txHash, rawTx, n.host.ID())


NOTE: @charithabandi: now taking an approach with a closure that captures the host's p2p ID rather than having CE/BP be aware.

jchappelow · 2025-01-14T14:52:19Z

Ready for review from @charithabandi , although after a day of testing and working out bugs, there are a few TODOs on this PR I intend to resolve.

This makes consensus parameters mutable and add the update and proposal code to support changing them. The RPCs and CLI commands may be removed for product, but they help to test the internals e2e. Part of this change includes changes related to key type, signers, authenticators, etc. Updates are made using the resolution system, with the new resolution type consensus.ParamUpdatesResolution. There are consensus cli commands and RPCs to submit and approve resolutions, show current params, and list proposed changes.

jchappelow force-pushed the paramupdates branch from 420f1c4 to 5cf608f Compare January 13, 2025 01:18

jchappelow commented Jan 13, 2025

View reviewed changes

jchappelow force-pushed the paramupdates branch 3 times, most recently from cbe3e9a to b269fbe Compare January 13, 2025 22:06

jchappelow marked this pull request as ready for review January 14, 2025 14:51

jchappelow requested a review from charithabandi January 14, 2025 14:52

jchappelow force-pushed the paramupdates branch from c2a5d5b to 716e88f Compare January 14, 2025 17:45

charithabandi approved these changes Jan 14, 2025

View reviewed changes

jchappelow mentioned this pull request Jan 14, 2025

hooks for consensus / network parameters #1227

Open

jchappelow added 2 commits January 14, 2025 11:48

clean out _previous

3bbad3e

jchappelow force-pushed the paramupdates branch from 716e88f to 3bbad3e Compare January 14, 2025 17:51

charithabandi merged commit ace8c40 into kwilteam:main Jan 14, 2025
2 checks passed

jchappelow deleted the paramupdates branch January 14, 2025 17:55

charithabandi linked an issue Jan 17, 2025 that may be closed by this pull request

consensus parameter storage and updates #1198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mutable consensus parameters #1213

mutable consensus parameters #1213

jchappelow commented Jan 10, 2025 •

edited

Loading

jchappelow left a comment

jchappelow Jan 13, 2025

jchappelow Jan 13, 2025

charithabandi Jan 14, 2025

jchappelow Jan 13, 2025

jchappelow Jan 13, 2025 •

edited

Loading

charithabandi Jan 14, 2025

jchappelow Jan 13, 2025

jchappelow Jan 13, 2025

jchappelow Jan 13, 2025 •

edited

Loading

charithabandi Jan 14, 2025

jchappelow Jan 14, 2025

jchappelow Jan 13, 2025

jchappelow Jan 13, 2025 •

edited

Loading

jchappelow commented Jan 14, 2025

		NetworkParameters *types.NetworkParameters

		NetworkUpdates types.ParamUpdates

		inMigration := bp.chainCtx.NetworkParameters.MigrationStatus == ktypes.MigrationInProgress
		haltNetwork := bp.chainCtx.NetworkParameters.MigrationStatus == ktypes.MigrationCompleted

		TxAnnouncer: func(ctx context.Context, txHash types.Hash, rawTx []byte) {
		n.announceTx(ctx, txHash, rawTx, n.host.ID())

mutable consensus parameters #1213

mutable consensus parameters #1213

Conversation

jchappelow commented Jan 10, 2025 • edited Loading

jchappelow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

jchappelow commented Jan 14, 2025

jchappelow commented Jan 10, 2025 •

edited

Loading

jchappelow Jan 13, 2025 •

edited

Loading

jchappelow Jan 13, 2025 •

edited

Loading

jchappelow Jan 13, 2025 •

edited

Loading