rpc: enforce BIP 145 SegWit rules in getblocktemplate

[TechLateef]

Change Description

This PR addresses issue #1738 by enforcing BIP 145 SegWit rules within the getblocktemplate RPC response.

Specifically, this PR updates rpcserver.go to ensure that:

• Client requests are rejected with ErrRPCInvalidParameter if SegWit is active but the client does not explicitly include the segwit rule in their request.
• The rules array in the response conditionally includes !segwit if the generated block template contains transactions with witness data (indicating a witness commitment is required).
• The rules array includes segwit (without the ! prefix) when SegWit is active but the template does not contain any witness transactions.

Steps to Test

1. Run the RPC server integration tests using go test -v -tags=rpctest -run=TestRpcServer ./integration
2. Ensure the newly added testGetBlockTemplateSegwitActiveNoRule, testGetBlockTemplateSegwitActiveWithRule, and testGetBlockTemplateResponseRules tests pass successfully.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.

[allocz]

Approach looks good, reversing rpcserver.go causes all the three added integration tests to fail, and confirmed the behavior of getblocktemplate against Bitcoin Core.

There's a broken test here:

=== RUN   TestHelp
    rpcserverhelp_test.go:43: Failed to generate help for method 'getblocktemplate': getblocktemplateresult-rules
--- FAIL: TestHelp (0.00s)

There's also some formatting changes and minor code readability improvements in the diff below. I know that not all the code is following the formatting rules, but I think that would be nice to at least follow them in the lines we modify. Thanks for the PR.

diff --git a/integration/rpcserver_test.go b/integration/rpcserver_test.go
index c7f52cef..ee34f173 100644
--- a/integration/rpcserver_test.go
+++ b/integration/rpcserver_test.go
@@ -299,8 +299,12 @@ func ensureSegwitActive(r *rpctest.Harness, t *testing.T) {
                        t.Fatalf("unable to get blockchain info: %v", err)
                }

-               if info.Bip9SoftForks == nil || info.Bip9SoftForks["segwit"] == nil {
-                       t.Fatalf("segwit softfork status not found in blockchain info")
+               if info.Bip9SoftForks == nil ||
+                       info.Bip9SoftForks["segwit"] == nil {
+
+                       // Segwit softfork status should be found in blockchain
+                       // info.
+                       t.Fatal("Missing segwit status on blockchain info")
                }

                status := info.Bip9SoftForks["segwit"].Status
@@ -309,7 +313,8 @@ func ensureSegwitActive(r *rpctest.Harness, t *testing.T) {
                }

                if _, err := r.Client.Generate(100); err != nil {
-                       t.Fatalf("unable to generate blocks to activate segwit: %v", err)
+                       t.Fatal("block generation to activate segwit failed;",
+                               "err:", err.Error())
                }
        }
 }
@@ -324,30 +329,33 @@ func testGetBlockTemplateSegwitActiveNoRule(r *rpctest.Harness, t *testing.T) {
        }

        _, err := r.Client.GetBlockTemplate(req)
-
        if err == nil {
-               t.Fatalf("Expected getblocktemplate to fail without 'segwit' rule")
+               t.Fatal("getblocktemplate must fail without 'segwit' rule")
        }

        // Expect: ErrRPCInvalidParameter with correct message
        rpcErr, ok := err.(*btcjson.RPCError)
-
        if !ok {
                t.Fatalf("Expected an RPCError, but got: %v", err)
        }
-
        if rpcErr.Code != btcjson.ErrRPCInvalidParameter {
-               t.Fatalf("Expected error code %d, but got: %d", btcjson.ErrRPCInvalidParameter, rpcErr.Code)
+               t.Fatal("Unexpected error code;",
+                       "expected", btcjson.ErrRPCInvalidParameter,
+                       "got", rpcErr.Code)
        }

-       expectedMessage := "Support for 'segwit' rule requires explicit client support"
-
+       expectedMessage := "Support for 'segwit' rule requires explicit " +
+               "client support"
        if rpcErr.Message != expectedMessage {
-               t.Fatalf("Expected error message '%s', but got: '%s'", expectedMessage, rpcErr.Message)
+               t.Fatal("Unexpected error message;",
+                       "expected", expectedMessage,
+                       "got", rpcErr.Message)
        }
 }

-func testGetBlockTemplateSegwitActiveWithRule(r *rpctest.Harness, t *testing.T) {
+func testGetBlockTemplateSegwitActiveWithRule(r *rpctest.Harness,
+       t *testing.T) {
+
        // Guarantee SegWit is fully active before testing
        ensureSegwitActive(r, t)

@@ -357,11 +365,10 @@ func testGetBlockTemplateSegwitActiveWithRule(r *rpctest.Harness, t *testing.T)
        }

        result, err := r.Client.GetBlockTemplate(req)
-
        if err != nil {
-               t.Fatalf("Expected getblocktemplate to succeed, got error: %v", err)
+               t.Fatal("Expected getblocktemplate to succeed;",
+                       "got error:", err.Error())
        }
-
        if result == nil {
                t.Fatal("Expected non-nil result")
        }
@@ -373,7 +380,6 @@ func testGetBlockTemplateSegwitActiveWithRule(r *rpctest.Harness, t *testing.T)
                        break
                }
        }
-
        if !hasSegwitRule {
                t.Fatalf("Expected 'segwit' rule to be present in the response")
        }
@@ -383,26 +389,25 @@ func testGetBlockTemplateResponseRules(r *rpctest.Harness, t *testing.T) {
        // Guarantee SegWit is fully active before testing
        ensureSegwitActive(r, t)

-       // Call getblocktemplate with 'segwit' rule when segwit is active
+       // Call getblocktemplate with 'segwit' rule when segwit is active.
        req := &btcjson.TemplateRequest{
                Rules: []string{"segwit"},
        }

        result, err := r.Client.GetBlockTemplate(req)
-
        if err != nil {
-               t.Fatalf("Expected getblocktemplate to succeed, got error: %v", err)
+               t.Fatal("Expected getblocktemplate to succeed;",
+                       "got error:", err.Error())
        }
-
        if result == nil {
                t.Fatal("Expected non-nil result")
        }

-       // Verify blockTemplateResult includes "!segwit" in Rules when WitnessCommitment is non-nil
-       // and "segwit" when WitnessCommitment is nil.
+       // Verify blockTemplateResult includes "!segwit" in Rules when
+       // WitnessCommitment is non-nil and "segwit" when WitnessCommitment is
+       // nil.
        hasNotSegwit := false
        hasSegwit := false
-
        for _, rule := range result.Rules {
                if rule == "!segwit" {
                        hasNotSegwit = true
@@ -410,15 +415,18 @@ func testGetBlockTemplateResponseRules(r *rpctest.Harness, t *testing.T) {
                        hasSegwit = true
                }
        }
+       hasWitnessCommitment := result.DefaultWitnessCommitment != ""
+       switch {
+       case hasWitnessCommitment && !hasNotSegwit:
+               // We expect Rules to contain '!segwit' when WitnessCommitment
+               // present.
+               t.Fatal("expected Rules to contain !segwit")
+
+       case !hasWitnessCommitment && !hasSegwit:
+               // We expect Rules to contain 'segwit' when WitnessCommitment is
+               // not present.
+               t.Fatal("expected Rules to contain segwit")

-       if result.DefaultWitnessCommitment != "" {
-               if !hasNotSegwit {
-                       t.Fatalf("Expected Rules to contain '!segwit' because WitnessCommitment is present")
-               }
-       } else {
-               if !hasSegwit {
-                       t.Fatalf("Expected Rules to contain 'segwit' because WitnessCommitment is absent")
-               }
        }
 }

diff --git a/rpcserver.go b/rpcserver.go
index b3faa90b..5ef652bc 100644
--- a/rpcserver.go
+++ b/rpcserver.go
@@ -20,6 +20,7 @@ import (
        "net"
        "net/http"
        "os"
+       "slices"
        "strconv"
        "strings"
        "sync"
@@ -1688,7 +1689,10 @@ func (state *gbtWorkState) updateBlockTemplate(s *rpcServer, useCoinbaseValue bo
 // and returned to the caller.
 //
 // This function MUST be called with the state locked.
-func (state *gbtWorkState) blockTemplateResult(useCoinbaseValue bool, submitOld *bool, segwitActive bool) (*btcjson.GetBlockTemplateResult, error) {
+func (state *gbtWorkState) blockTemplateResult(useCoinbaseValue,
+       segwitActive bool,
+       submitOld *bool) (*btcjson.GetBlockTemplateResult, error) {
+
        // Ensure the timestamps are still in valid range for the template.
        // This should really only ever happen if the local clock is changed
        // after the template is generated, but it's important to avoid serving
@@ -1843,7 +1847,10 @@ func (state *gbtWorkState) blockTemplateResult(useCoinbaseValue bool, submitOld
 // has passed without finding a solution.
 //
 // See https://en.bitcoin.it/wiki/BIP_0022 for more details.
-func handleGetBlockTemplateLongPoll(s *rpcServer, longPollID string, useCoinbaseValue bool, closeChan <-chan struct{}, segwitActive bool) (interface{}, error) {
+func handleGetBlockTemplateLongPoll(s *rpcServer, longPollID string,
+       useCoinbaseValue, segwitActive bool,
+       closeChan <-chan struct{}) (interface{}, error) {
+
        state := s.gbtWorkState
        state.Lock()
        // The state unlock is intentionally not deferred here since it needs to
@@ -1859,7 +1866,9 @@ func handleGetBlockTemplateLongPoll(s *rpcServer, longPollID string, useCoinbase
        // the caller is invalid.
        prevHash, lastGenerated, err := decodeTemplateID(longPollID)
        if err != nil {
-               result, err := state.blockTemplateResult(useCoinbaseValue, nil, segwitActive)
+               result, err := state.blockTemplateResult(
+                       useCoinbaseValue, segwitActive, nil,
+               )
                if err != nil {
                        state.Unlock()
                        return nil, err
@@ -1880,8 +1889,9 @@ func handleGetBlockTemplateLongPoll(s *rpcServer, longPollID string, useCoinbase
                // old block template depending on whether or not a solution has
                // already been found and added to the block chain.
                submitOld := prevHash.IsEqual(prevTemplateHash)
-               result, err := state.blockTemplateResult(useCoinbaseValue,
-                       &submitOld, segwitActive)
+               result, err := state.blockTemplateResult(
+                       useCoinbaseValue, segwitActive, &submitOld,
+               )
                if err != nil {
                        state.Unlock()
                        return nil, err
@@ -1921,7 +1931,9 @@ func handleGetBlockTemplateLongPoll(s *rpcServer, longPollID string, useCoinbase
        // block template depending on whether or not a solution has already
        // been found and added to the block chain.
        submitOld := prevHash.IsEqual(&state.template.Block.Header.PrevBlock)
-       result, err := state.blockTemplateResult(useCoinbaseValue, &submitOld, segwitActive)
+       result, err := state.blockTemplateResult(
+               useCoinbaseValue, segwitActive, &submitOld,
+       )
        if err != nil {
                return nil, err
        }
@@ -1990,27 +2002,21 @@ func handleGetBlockTemplateRequest(s *rpcServer, request *btcjson.TemplateReques
                }
        }

-       segwitState, err := s.cfg.Chain.ThresholdState(chaincfg.DeploymentSegwit)
+       segwitState, err := s.cfg.Chain.ThresholdState(
+               chaincfg.DeploymentSegwit,
+       )
        if err != nil {
                return nil, err
        }

        segwitActive := segwitState == blockchain.ThresholdActive
-       hasSegwitRule := false
-       if request != nil {
-               for _, rule := range request.Rules {
-                       if rule == "segwit" {
-                               hasSegwitRule = true
-                               break
-                       }
-               }
-       }
-
+       hasSegwitRule := request != nil &&
+               slices.Contains(request.Rules, "segwit")
        if segwitActive && !hasSegwitRule {
                return nil, &btcjson.RPCError{
                        Code: btcjson.ErrRPCInvalidParameter,
-                       Message: "Support for 'segwit' rule requires explicit " +
-                               "client support",
+                       Message: "Support for 'segwit' rule requires " +
+                               "explicit client support",
                }
        }

@@ -2018,8 +2024,10 @@ func handleGetBlockTemplateRequest(s *rpcServer, request *btcjson.TemplateReques
        // client to be notified when block template referenced by the ID should
        // be replaced with a new one.
        if request != nil && request.LongPollID != "" {
-               return handleGetBlockTemplateLongPoll(s, request.LongPollID,
-                       useCoinbaseValue, closeChan, segwitActive)
+               return handleGetBlockTemplateLongPoll(
+                       s, request.LongPollID, useCoinbaseValue, segwitActive,
+                       closeChan,
+               )
        }

        // Protect concurrent access when updating block templates.
@@ -2036,7 +2044,9 @@ func handleGetBlockTemplateRequest(s *rpcServer, request *btcjson.TemplateReques
        if err := state.updateBlockTemplate(s, useCoinbaseValue); err != nil {
                return nil, err
        }
-       return state.blockTemplateResult(useCoinbaseValue, nil, segwitActive)
+       return state.blockTemplateResult(
+               useCoinbaseValue, segwitActive, nil,
+       )
 }

 // chainErrToGBTErrString converts an error returned from btcchain to a string