feat: strengthen input validation #164
Conversation
…d strict hex checks; add tests
There was a problem hiding this comment.
Pull Request Overview
This PR strengthens input validation across the staking system by adding integer checks for numeric parameters, validating covenant signature uniqueness, and implementing strict hex format validation for slashing scripts. The changes prevent malformed inputs that could cause crashes or produce invalid transactions.
- Adds integer validation for staking amounts, timelock periods, and fee rates
- Enforces covenant signature uniqueness to prevent duplicate signatures from the same public key
- Implements comprehensive hex format validation for slashing script parameters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/staking/validation.ts |
Adds integer validation checks for stakingAmountSat, timelock, and feeRate parameters |
src/staking/transactions.ts |
Implements hex format validation for slashingPkScriptHex and covenant signature uniqueness enforcement |
tests/utils/staking/validation.test.ts |
Adds test cases for integer validation errors |
tests/staking/transactions/slashingTransaction.test.ts |
Adds test cases for slashingPkScriptHex format validation |
tests/staking/createCovenantWitness.test.ts |
Adds test case for duplicate covenant signature detection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
gbarkhatov
left a comment
gbarkhatov
left a comment
There was a problem hiding this comment.
I think most of the Copilot comments are valid, otherwise seems good
|
need rebase and fix all comments. @jonybur |
…jb-strengthen-input-validation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return paramsCovenants.some((covenant) => | ||
| covenant.equals(btcPkHexBuf as Uint8Array), | ||
| ); |
There was a problem hiding this comment.
The type cast as Uint8Array is unnecessary. Buffer already extends Uint8Array in Node.js, and the equals method accepts Buffer directly. Remove the cast for cleaner code.
| // we need covenant from params to be sorted in reverse order | ||
| const paramsCovenantsSorted = [...paramsCovenants] | ||
| .sort(Buffer.compare) | ||
| .sort((a, b) => Buffer.compare(a as Uint8Array, b as Uint8Array)) |
There was a problem hiding this comment.
The type casts as Uint8Array are unnecessary. Buffer.compare accepts Buffer arguments directly (as seen in src/staking/stakingScript.ts:295 where it's used without casts). Remove the casts: .sort(Buffer.compare)
| .sort((a, b) => Buffer.compare(a as Uint8Array, b as Uint8Array)) | |
| .sort(Buffer.compare) |
| // otherwise we return empty Buffer | ||
| const covenantSig = covenantSigsBuffers.find( | ||
| (sig) => sig.btcPkHex.compare(covenant) === 0, | ||
| (sig) => covenant.compare(sig.btcPkHex as Uint8Array) === 0, |
There was a problem hiding this comment.
The type cast as Uint8Array is unnecessary. Buffer.compare accepts Buffer arguments directly. Remove the cast for consistency with the rest of the codebase.
| if (slashingOutput.length < 1) { | ||
| throw new Error( | ||
| "slashingPkScriptHex decoded to an empty buffer (expected at least 1 byte)", | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This check is redundant. Lines 738-743 already validate that slashingPkScriptHex is non-empty and has even length. An empty string would be caught by slashingPkScriptHex.length === 0, and any valid non-empty hex string with even length will decode to at least 1 byte. This validation can be safely removed.
| if (slashingOutput.length < 1) { | |
| throw new Error( | |
| "slashingPkScriptHex decoded to an empty buffer (expected at least 1 byte)", | |
| ); | |
| } |
| if (!Number.isInteger(stakingAmountSat)) { | ||
| throw new StakingError( | ||
| StakingErrorCode.INVALID_INPUT, | ||
| "Invalid staking amount", | ||
| ); | ||
| } |
There was a problem hiding this comment.
The error message 'Invalid staking amount' is ambiguous and duplicated at line 135 for range validation. Consider making this more specific, such as 'Staking amount must be an integer' to help users understand what's wrong.
| if (!Number.isInteger(timelock)) { | ||
| throw new StakingError(StakingErrorCode.INVALID_INPUT, "Invalid timelock"); | ||
| } |
There was a problem hiding this comment.
The error message 'Invalid timelock' is ambiguous and duplicated at line 143 for range validation. Consider making this more specific, such as 'Timelock must be an integer' to help users understand what's wrong.
| if (!Number.isInteger(feeRate)) { | ||
| throw new StakingError(StakingErrorCode.INVALID_INPUT, "Invalid fee rate"); | ||
| } |
There was a problem hiding this comment.
The error message 'Invalid fee rate' is ambiguous and duplicated at line 153 for positive value validation. Consider making this more specific, such as 'Fee rate must be an integer' to help users understand what's wrong.
|
@jonybur fyi there are quite some AI comments |
Adds input validation: integer checks, covenant signature uniqueness with correct mapping, and strict slashingPkScriptHex validation. Include tests. Prevents malformed inputs, and avoids crashes or invalid transactions.
#163