standalone: Introduce CalcTSpendWindow.

This existing code for calculating treasury spend window values, namely
CalculateTSpendWindowStart and CalculateTSpendWindowEnd, has a couple of
footguns:

- The starting value calculation returns totally invalid values without
  an error when provided with a expiry that is prior to the first voting
  window
- Expiry values that are invalid for the start calc are valid for the
  end calc even though they should not be

As a result, the code for handling the error paths in the rpc server and
mining code produces error messages that don't make sense in some cases
because they blindly print the start value without checking the error.

To remove the aforementioned footguns and also address the incorrect
error messages, the following overview of changes are made:

- Replaces the CalculateTSpendWindowStart and CalculateTSpendWindowEnd
  functions with a single combined CalcTSpendWindow function that
  properly checks for expiries before the first possible voting interval
- Modifies the returned errors to match the new reality
- Shortens the prefix to Calc to match the rest of the function names in
  the package
- Updates the incorrect error handling paths to avoid reporting the
  values that are not guaranteed to be meaningful and modifies the error
  messages to be more consistent
- Adds a comprehensive set of tests for the new function

Note that in addition to addressing the aforementioned issues, this
approach has at least a couple of other benefits:

- It is more efficient to have a single function with a single bounds
  check
- Almost all callers outside of the tests need both the start and end
  value anyway, so it allows more compact code

Finally, this updates all callers in the repository accordingly.
This commit is contained in:
Dave Collins 2020-09-27 15:53:59 -05:00
parent e6c0a43e84
commit bf57746c3b
No known key found for this signature in database
GPG Key ID: B8904D9D9C93D1F2
11 changed files with 153 additions and 127 deletions

View File

@ -20,13 +20,9 @@ const (
// lower than the required target difficultly.
ErrHighHash = ErrorKind("ErrHighHash")
// ErrTSpendStartInvalidExpiry indicates that an invalid expiry was
// provided to calculate the start of a treasury spend vote window.
ErrTSpendStartInvalidExpiry = ErrorKind("ErrTSpendStartInvalidExpiry")
// ErrTSpendEndInvalidExpiry indicates that an invalid expiry was
// provided to calculate the end of a treasury spend vote window.
ErrTSpendEndInvalidExpiry = ErrorKind("ErrTSpendEndInvalidExpiry")
// ErrInvalidTSpendExpiry indicates that an invalid expiry was
// provided when calculating the treasury spend voting window.
ErrInvalidTSpendExpiry = ErrorKind("ErrInvalidTSpendExpiry")
)
// Error satisfies the error interface and prints human-readable errors.

View File

@ -18,8 +18,7 @@ func TestErrorKindStringer(t *testing.T) {
}{
{ErrUnexpectedDifficulty, "ErrUnexpectedDifficulty"},
{ErrHighHash, "ErrHighHash"},
{ErrTSpendStartInvalidExpiry, "ErrTSpendStartInvalidExpiry"},
{ErrTSpendEndInvalidExpiry, "ErrTSpendEndInvalidExpiry"},
{ErrInvalidTSpendExpiry, "ErrInvalidTSpendExpiry"},
}
t.Logf("Running %d tests", len(tests))

View File

@ -24,26 +24,29 @@ func IsTreasuryVoteInterval(height, tvi uint64) bool {
return height%tvi == 0 && height != 0
}
// CalculateTSpendWindowStart calculates the start of a treasury voting window
// based on the parameters that are passed. Great care must be taken to ensure
// this function is only called with an expiry that *IS* on a TVI.
func CalculateTSpendWindowStart(expiry uint32, tvi, multiplier uint64) (uint32, error) {
if !IsTreasuryVoteInterval(uint64(expiry-2), tvi) {
return 0, ruleError(ErrTSpendStartInvalidExpiry,
fmt.Sprintf("invalid start expiry: %v", expiry))
// CalcTSpendWindow calculates the start and end of a treasury voting window
// based on the parameters that are passed. An error will be returned if the
// provided expiry is not two more than a treasury vote interval (TVI) or before
// a single voting window is possible.
func CalcTSpendWindow(expiry uint32, tvi, multiplier uint64) (uint32, uint32, error) {
// Ensure the provided expiry is at least higher than a single voting
// window.
minReqExpiry := tvi*multiplier + 2
if uint64(expiry) < minReqExpiry {
str := fmt.Sprintf("expiry %d must be at least %d for the voting "+
"window defined by a TVI of %d with a multiplier of %d", expiry,
minReqExpiry, tvi, multiplier)
return 0, 0, ruleError(ErrInvalidTSpendExpiry, str)
}
return expiry - uint32(tvi*multiplier) - 2, nil
}
// CalculateTSpendWindowEnd calculates the end of a treasury voting window
// based on the parameters that are passed. Great care must be taken to ensure
// this function is only called with an expiry that *IS* on a TVI.
func CalculateTSpendWindowEnd(expiry uint32, tvi uint64) (uint32, error) {
// Ensure the provided expiry is two more than a TVI.
if !IsTreasuryVoteInterval(uint64(expiry-2), tvi) {
return 0, ruleError(ErrTSpendEndInvalidExpiry,
fmt.Sprintf("invalid end expiry: %v", expiry))
str := fmt.Sprintf("expiry %d must be two more than a multiple of the "+
"treasury vote interval %d", expiry, tvi)
return 0, 0, ruleError(ErrInvalidTSpendExpiry, str)
}
return expiry - 2, nil
return expiry - uint32(tvi*multiplier) - 2, expiry - 2, nil
}
// InsideTSpendWindow returns true if the provided block height is inside the
@ -55,13 +58,10 @@ func CalculateTSpendWindowEnd(expiry uint32, tvi uint64) (uint32, error) {
// Note: The end is INCLUSIVE in order to determine if a TSPEND is allowed in a
// block despite the fact that voting window is EXCLUSIVE.
func InsideTSpendWindow(blockHeight int64, expiry uint32, tvi, multiplier uint64) bool {
s, err := CalculateTSpendWindowStart(expiry, tvi, multiplier)
start, end, err := CalcTSpendWindow(expiry, tvi, multiplier)
if err != nil {
return false
}
e, err := CalculateTSpendWindowEnd(expiry, tvi)
if err != nil {
return false
}
return uint32(blockHeight) >= s && uint32(blockHeight) <= e
return uint32(blockHeight) >= start && uint32(blockHeight) <= end
}

View File

@ -9,19 +9,106 @@ import (
"testing"
)
func TestTSpendExpiryNegative(t *testing.T) {
// 5 is not a valid start for a tvi of 11 with a mul of 3.
_, err := CalculateTSpendWindowStart(5, 11, 3)
if !errors.Is(err, ErrTSpendStartInvalidExpiry) {
t.Fatalf("expected %v got %v",
ErrTSpendStartInvalidExpiry, err)
}
const (
// mainNetTVI is the treasury vote interval for mainnet.
mainNetTVI = 288
// 5 is not a valid end for a tvi of 11.
_, err = CalculateTSpendWindowEnd(5, 11)
if !errors.Is(err, ErrTSpendEndInvalidExpiry) {
t.Fatalf("expected %v got %v",
ErrTSpendEndInvalidExpiry, err)
// mainNetTVIMul is the treasury vote interval multiplier for mainnet.
mainNetTVIMul = 12
)
// TestCalcTSpendWindow ensures that the function that calculates the start and
// end of a treasury spend voting window returns the expected results including
// error conditions.
func TestCalcTSpendWindow(t *testing.T) {
tests := []struct {
name string // test description
expiry uint32 // expiry to calculate the window for
tvi uint64 // treasury vote interval
tvimul uint64 // treasury vote interval multiplier
err error // expected error
wantStart uint32 // expected start result
wantEnd uint32 // expected end result
}{{
name: "zero is not a valid expiry",
expiry: 0,
tvi: mainNetTVI,
tvimul: mainNetTVIMul,
err: ErrInvalidTSpendExpiry,
wantStart: 0,
wantEnd: 0,
}, {
name: "min required expiry - 1",
expiry: mainNetTVI*mainNetTVIMul + 1,
tvi: mainNetTVI,
tvimul: mainNetTVIMul,
err: ErrInvalidTSpendExpiry,
wantStart: 0,
wantEnd: 0,
}, {
name: "not a TVI + 2",
expiry: mainNetTVI*mainNetTVIMul + 3,
tvi: mainNetTVI,
tvimul: mainNetTVIMul,
err: ErrInvalidTSpendExpiry,
wantStart: 0,
wantEnd: 0,
}, {
name: "5 is not a valid start or end for a tvi 11, mul 3",
expiry: 5,
tvi: 11,
tvimul: 3,
err: ErrInvalidTSpendExpiry,
wantStart: 0,
wantEnd: 0,
}, {
name: "first possible valid mainnet params",
expiry: mainNetTVI*mainNetTVIMul + 2,
tvi: mainNetTVI,
tvimul: mainNetTVIMul,
err: nil,
wantStart: 0,
wantEnd: mainNetTVI * mainNetTVIMul,
}, {
name: "second possible valid mainnet params",
expiry: mainNetTVI*mainNetTVIMul*2 + 2,
tvi: mainNetTVI,
tvimul: mainNetTVIMul,
err: nil,
wantStart: mainNetTVI * mainNetTVIMul,
wantEnd: mainNetTVI * mainNetTVIMul * 2,
}, {
name: "5186 for tvi 288, mul 7 is window [3168, 5184)",
expiry: 5186,
tvi: 288,
tvimul: 7,
err: nil,
wantStart: 5186 - 288*7 - 2,
wantEnd: 5186 - 2,
}}
for _, test := range tests {
// Calculate result and ensure the expected error is produced.
start, end, err := CalcTSpendWindow(test.expiry, test.tvi, test.tvimul)
if !errors.Is(err, test.err) {
t.Errorf("%q: unexpected error -- got %v, want %v", test.name, err,
test.err)
continue
}
// Ensure the expected start value is calculated.
if start != test.wantStart {
t.Errorf("%q: unexpected start val -- got %v, want %v", test.name,
start, test.wantStart)
continue
}
// Ensure the expected end value is calculated.
if end != test.wantEnd {
t.Errorf("%q: unexpected end val -- got %v, want %v", test.name,
end, test.wantEnd)
continue
}
}
}
@ -32,32 +119,4 @@ func TestTSpendExpiry(t *testing.T) {
if expiry != 5186 {
t.Fatalf("got %v, expected 5186", expiry)
}
start, err := CalculateTSpendWindowStart(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
end, err := CalculateTSpendWindowEnd(expiry, tvi)
if err != nil {
t.Fatal(err)
}
// Expect 3168 = expiry - (tvi*mul) - 2
expectedStart := expiry - uint32(tvi*mul) - 2
if start != expectedStart {
t.Fatalf("got %v, expected %v", start, expectedStart)
}
// Expect 5184 = expiry - 2
expectedEnd := expiry - 2
if end != expectedEnd {
t.Fatalf("got %v, expected %v", end, expectedEnd)
}
// Hard check numbers as well.
if expectedStart != 3168 {
t.Fatalf("got %v, expected %v", expectedStart, 3168)
}
if expectedEnd != 5184 {
t.Fatalf("got %v, expected %v", expectedEnd, 5184)
}
}

View File

@ -885,17 +885,12 @@ func (b *BlockChain) tSpendCountVotes(prevNode *blockNode, tspend *dcrutil.Tx) (
)
expiry := tspend.MsgTx().Expiry
t.start, err = standalone.CalculateTSpendWindowStart(expiry,
t.start, t.end, err = standalone.CalcTSpendWindow(expiry,
b.chainParams.TreasuryVoteInterval,
b.chainParams.TreasuryVoteIntervalMultiplier)
if err != nil {
return nil, err
}
t.end, err = standalone.CalculateTSpendWindowEnd(expiry,
b.chainParams.TreasuryVoteInterval)
if err != nil {
return nil, err
}
nextHeight := prevNode.height + 1
trsyLog.Tracef(" tSpendCountVotes: nextHeight %v start %v expiry %v",

View File

@ -523,11 +523,7 @@ func TestTSpendVoteCount(t *testing.T) {
tspendFee := 100
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
end, err := standalone.CalculateTSpendWindowEnd(expiry, tvi)
start, end, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -738,7 +734,7 @@ func TestTSpendVoteCount(t *testing.T) {
// Use exact hight to validate that tspend starts on next tvi.
expiry = standalone.CalculateTSpendExpiry(int64(g.Tip().Header.Height),
tvi, mul)
start, err = standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err = standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -946,7 +942,7 @@ func TestTSpendEmptyTreasury(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -1170,7 +1166,7 @@ func TestTSpendExpendituresPolicy(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -1661,7 +1657,7 @@ func TestExpendituresReorg(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi, mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -1869,7 +1865,7 @@ func TestSpendableTreasuryTxs(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -2164,7 +2160,7 @@ func TestTSpendDupVote(t *testing.T) {
// ---------------------------------------------------------------------
nextBlockHeight := g.Tip().Header.Height + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi, mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -2306,7 +2302,7 @@ func TestTSpendTooManyTSpend(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -2423,7 +2419,7 @@ func TestTSpendWindow(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height + uint32(tvi*mul*4) + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -2547,7 +2543,7 @@ func TestTSpendSignature(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height //+ uint32(tvi*mul*4) + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -2767,7 +2763,7 @@ func TestTSpendSignatureInvalid(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height //+ uint32(tvi*mul*4) + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -2965,7 +2961,7 @@ func TestTSpendExists(t *testing.T) {
nextBlockHeight := g.Tip().Header.Height + 1
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -3870,7 +3866,7 @@ func TestTSpendCorners(t *testing.T) {
tspendFee := 100
expiry := standalone.CalculateTSpendExpiry(int64(nextBlockHeight), tvi,
mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
start, _, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -3958,11 +3954,7 @@ func TestTSpendFirstTVICorner(t *testing.T) {
// total of 55 possible. That easily is voted in since 60% is 33 yes
// votes.
expiry := standalone.CalculateTSpendExpiry(140, tvi, mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
end, err := standalone.CalculateTSpendWindowEnd(expiry, tvi)
start, end, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
@ -4250,11 +4242,7 @@ func TestTSpendVoteCountSynthetic(t *testing.T) {
tpb := params.TicketsPerBlock
expiry := standalone.CalculateTSpendExpiry(int64(tmh), tvi, mul)
start, err := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}
end, err := standalone.CalculateTSpendWindowEnd(expiry, tvi)
start, end, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}

View File

@ -3432,20 +3432,15 @@ func (b *BlockChain) tspendChecks(prevNode *blockNode, block *dcrutil.Block) err
return ruleError(ErrNotTVI, str)
}
// Assert that the tspend is inside the correct window.
// Assert that the treasury spend is inside the correct window.
exp := stx.MsgTx().Expiry
if !standalone.InsideTSpendWindow(blockHeight,
exp, b.chainParams.TreasuryVoteInterval,
b.chainParams.TreasuryVoteIntervalMultiplier) {
s, _ := standalone.CalculateTSpendWindowStart(exp,
b.chainParams.TreasuryVoteInterval,
b.chainParams.TreasuryVoteIntervalMultiplier)
str := fmt.Sprintf("block contains TSpend "+
"transaction (%v) that is outside "+
"of the valid window: height %v "+
"start %v expiry %v",
stx.Hash(), block.Height(), s, exp)
str := fmt.Sprintf("block at height %d contains treasury spend "+
"transaction %v with expiry %v that is outside of the valid "+
"window", blockHeight, stx.Hash(), exp)
return ruleError(ErrInvalidTSpendWindow, str)
}

View File

@ -1630,8 +1630,7 @@ func (mp *TxPool) maybeAcceptTransaction(tx *dcrutil.Tx, isNew, rateLimit, allow
// its voting is supposed to start. We arbitrarily define as
// "too far in the future" as the vote starting greater than or
// equal to two full voting windows in the future.
voteStart, err := standalone.CalculateTSpendWindowStart(msgTx.Expiry,
tvi, mul)
voteStart, _, err := standalone.CalcTSpendWindow(msgTx.Expiry, tvi, mul)
if err != nil {
str := fmt.Sprintf("Invalid tspend expiry %d: %v ",
msgTx.Expiry, err)

View File

@ -2613,7 +2613,7 @@ func TestHandlesTSpends(t *testing.T) {
// when voting ends and advance the fake chain to just before that
// height. The tspend can be mined on the block the vote ends, which is
// a TVI block.
endVote, err := standalone.CalculateTSpendWindowEnd(expiry, tvi)
_, endVote, err := standalone.CalcTSpendWindow(expiry, tvi, mul)
if err != nil {
t.Fatal(err)
}

View File

@ -1461,13 +1461,9 @@ mempoolLoop:
exp, g.chainParams.TreasuryVoteInterval,
g.chainParams.TreasuryVoteIntervalMultiplier) {
s, _ := standalone.CalculateTSpendWindowStart(exp,
g.chainParams.TreasuryVoteInterval,
g.chainParams.TreasuryVoteIntervalMultiplier)
log.Tracef("Skipping tspend %v because it is "+
"outside of the window: height %v "+
"start %v expiry %v reason %v",
tx.Hash(), nextBlockHeight, s, exp, err)
log.Tracef("Skipping treasury spend %v at height %d because it "+
"has an expiry of %d that is outside of the voting window",
tx.Hash(), nextBlockHeight, exp)
continue
}

View File

@ -3323,10 +3323,9 @@ func handleGetTreasurySpendVotes(_ context.Context, s *Server, cmd interface{})
}
}
// The following errors can be ignored because we checked the
// expiry is in a TVI earlier.
start, _ := standalone.CalculateTSpendWindowStart(expiry, tvi, mul)
end, _ := standalone.CalculateTSpendWindowEnd(expiry, tvi)
// The following error can be ignored because the expiry was verified to
// be in a TVI earlier.
start, end, _ := standalone.CalcTSpendWindow(expiry, tvi, mul)
votes[i] = types.TreasurySpendVotes{
Hash: txHash.String(),