Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly validate whether tranactions are standard #185

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

KonradStaniec
Copy link
Collaborator

Add:

@KonradStaniec KonradStaniec requested a review from a team as a code owner October 11, 2024 13:14
@KonradStaniec KonradStaniec requested review from gitferry and samricotta and removed request for a team October 11, 2024 13:14
@KonradStaniec KonradStaniec added consensus breaking change modifies `appHash` of the application backport release/v0.12.x labels Oct 11, 2024
Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work

@@ -413,3 +413,148 @@ func TestNotAllowFinalityProviderKeysAsCovenantKeys(t *testing.T) {
require.Error(t, err)
require.True(t, errors.Is(err, btcstaking.ErrDuplicatedKeyInScript))
}

func TestCheckPreSignedTxSanity(t *testing.T) {
tests := []struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Parallel()

}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := btcstaking.CheckPreSignedTxSanity(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Parallel() in subtest also

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be backported or it can go to the next version?

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@@ -215,6 +223,89 @@ func IsSimpleTransfer(tx *wire.MsgTx) error {
return nil
}

// CheckPreSignedTxSanity perfomorms basic checks on a pre-signed transaction:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// CheckPreSignedTxSanity perfomorms basic checks on a pre-signed transaction:
// CheckPreSignedTxSanity performs basic checks on a pre-signed transaction:

@KonradStaniec
Copy link
Collaborator Author

Does this need to be backported or it can go to the next version?

I wanted to backport it to release patch version v0.12.1, and bump phase-1 services which use this validation: staking-api, and covenant-emulator.

Indexer does not need it as it reads data from BTC ledger, so if miner inserted v0 transaction into the ledger, then indexer must process it

@KonradStaniec KonradStaniec merged commit 508046b into main Oct 14, 2024
20 checks passed
@KonradStaniec KonradStaniec deleted the konradstaniec/fix-accepting-non-standard-tx branch October 14, 2024 05:53
KonradStaniec added a commit that referenced this pull request Oct 14, 2024
Add:
- new `btcstaking` library function to validate whether transactions are
standard based on
https://github.com/btcsuite/btcd/blob/ee68dc66a835bf2c9333f3d7b33791841f561c84/mempool/policy.go#L285
- use those function to validate unbonding / slashing transactions
- test suite to test new functions
KonradStaniec added a commit that referenced this pull request Oct 14, 2024
Add:
- new `btcstaking` library function to validate whether transactions are
standard based on

https://github.com/btcsuite/btcd/blob/ee68dc66a835bf2c9333f3d7b33791841f561c84/mempool/policy.go#L285
- use those function to validate unbonding / slashing transactions
- test suite to test new functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release/v0.12.x consensus breaking change modifies `appHash` of the application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants