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.

Sign up for GitHub

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

P-chain - Memo field zeroed post Durango #2607

Merged
merged 18 commits into from
Jan 19, 2024
Merged

P-chain - Memo field zeroed post Durango #2607

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d01b7ee
forbid memo field in durango
abi87 Jan 13, 2024
3181e9a
nit
abi87 Jan 13, 2024
5bbe14c
nit
abi87 Jan 13, 2024
46de238
Merge branch 'dev' into p-chain-no-memo-field-durango
abi87 Jan 15, 2024
6854972
Merge branch 'dev' into p-chain-no-memo-field-durango
abi87 Jan 16, 2024
80332a3
Merge branch 'dev' into p-chain-no-memo-field-durango
abi87 Jan 17, 2024
9b5ac0a
Merge branch 'dev' into p-chain-no-memo-field-durango
abi87 Jan 18, 2024
c0ce5ad
nits
abi87 Jan 19, 2024
53e9a44
nits
abi87 Jan 19, 2024
b8df184
wip: adding UTs for memo length
abi87 Jan 19, 2024
1fdf681
wip: adding UTs for memo length
abi87 Jan 19, 2024
1b01c22
nits
abi87 Jan 19, 2024
3d33013
added UT
abi87 Jan 19, 2024
dcf5591
Merge branch 'master' into p-chain-no-memo-field-durango
abi87 Jan 19, 2024
1e10b39
nits
abi87 Jan 19, 2024
557bfb9
Update vms/components/avax/base_tx.go
StephenButtolph Jan 19, 2024
99e56de
Merge branch 'master' into p-chain-no-memo-field-durango
StephenButtolph Jan 19, 2024
6009d83
nits (#2640)
dhrubabasu Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions vms/components/avax/base_tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,20 @@ func (t *BaseTx) Verify(ctx *snow.Context) error {
return nil
}
}

func VerifyMemoFieldLength(memo types.JSONByteSlice, isDurangoActive bool) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a different method rather than extending SyntacticVerify with config and chainTIme. We can revisit this choice down the line, in case other syntactic checks will depend on the specific fork used

if !isDurangoActive {
return nil // field already validated during in SyntacticVerify
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}

if len(memo) != 0 {
return fmt.Errorf(
"%w: %d > %d",
ErrMemoTooLarge,
len(memo),
0,
)
}

return nil
}
67 changes: 54 additions & 13 deletions vms/platformvm/txs/executor/staker_tx_verification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,16 @@ func verifyAddValidatorTx(
var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
startTime = currentTimestamp
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return nil, err
}

startTime := currentTimestamp
if !isDurangoActive {
startTime = tx.StartTime()
}

duration := tx.EndTime().Sub(startTime)
switch {
case tx.Validator.Wght < backend.Config.MinValidatorStake:
Expand Down Expand Up @@ -194,8 +199,12 @@ func verifyAddSubnetValidatorTx(
var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
startTime = currentTimestamp
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

startTime := currentTimestamp
if !isDurangoActive {
startTime = tx.StartTime()
}
Expand Down Expand Up @@ -283,6 +292,14 @@ func verifyRemoveSubnetValidatorTx(
return nil, false, err
}

var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return nil, false, err
}

isCurrentValidator := true
vdr, err := chainState.GetCurrentValidator(tx.Subnet, tx.NodeID)
if err == database.ErrNotFound {
Expand Down Expand Up @@ -351,8 +368,14 @@ func verifyAddDelegatorTx(
var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
endTime = tx.EndTime()
startTime = currentTimestamp
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return nil, err
}

var (
endTime = tx.EndTime()
startTime = currentTimestamp
)
if !isDurangoActive {
startTime = tx.StartTime()
Expand Down Expand Up @@ -458,15 +481,19 @@ func verifyAddPermissionlessValidatorTx(
return err
}

if !backend.Bootstrapped.Get() {
return nil
}

var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
startTime = currentTimestamp
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

if !backend.Bootstrapped.Get() {
return nil
}

startTime := currentTimestamp
if !isDurangoActive {
startTime = tx.StartTime()
}
Expand Down Expand Up @@ -578,15 +605,21 @@ func verifyAddPermissionlessDelegatorTx(
return err
}

var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

if !backend.Bootstrapped.Get() {
return nil
}

var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
endTime = tx.EndTime()
startTime = currentTimestamp
endTime = tx.EndTime()
startTime = currentTimestamp
)
if !isDurangoActive {
startTime = tx.StartTime()
Expand Down Expand Up @@ -728,6 +761,14 @@ func verifyTransferSubnetOwnershipTx(
return err
}

var (
currentTimestamp = chainState.GetTimestamp()
isDurangoActive = backend.Config.IsDurangoActivated(currentTimestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
StephenButtolph marked this conversation as resolved.
Show resolved Hide resolved
return err
}

if !backend.Bootstrapped.Get() {
// Not bootstrapped yet -- don't need to do full verification.
return nil
Expand Down
6 changes: 4 additions & 2 deletions vms/platformvm/txs/executor/staker_tx_verification_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,15 @@ func TestVerifyAddPermissionlessValidatorTx(t *testing.T) {
}
},
stateF: func(ctrl *gomock.Controller) state.Chain {
return nil
mockState := state.NewMockChain(ctrl)
mockState.EXPECT().GetTimestamp().Return(now) // chain time is after Durango fork activation since now.After(activeForkTime)
return mockState
},
sTxF: func() *txs.Tx {
return &verifiedSignedTx
},
txF: func() *txs.AddPermissionlessValidatorTx {
return nil
return &txs.AddPermissionlessValidatorTx{}
},
expectedErr: nil,
},
Expand Down
50 changes: 48 additions & 2 deletions vms/platformvm/txs/executor/standard_tx_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,20 @@ func (e *StandardTxExecutor) CreateChainTx(tx *txs.CreateChainTx) error {
return err
}

var (
timestamp = e.State.GetTimestamp()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: timestamp -> currentTimestamp to match the usage in other funcs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

isDurangoActive = e.Config.IsDurangoActivated(timestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

baseTxCreds, err := verifyPoASubnetAuthorization(e.Backend, e.State, e.Tx, tx.SubnetID, tx.SubnetAuth)
if err != nil {
return err
}

// Verify the flowcheck
timestamp := e.State.GetTimestamp()
createBlockchainTxFee := e.Config.GetCreateBlockchainTxFee(timestamp)
if err := e.FlowChecker.VerifySpend(
tx,
Expand Down Expand Up @@ -98,8 +105,15 @@ func (e *StandardTxExecutor) CreateSubnetTx(tx *txs.CreateSubnetTx) error {
return err
}

var (
timestamp = e.State.GetTimestamp()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: timestamp -> currentTimestamp to match the usage in other funcs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

isDurangoActive = e.Config.IsDurangoActivated(timestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

// Verify the flowcheck
timestamp := e.State.GetTimestamp()
createSubnetTxFee := e.Config.GetCreateSubnetTxFee(timestamp)
if err := e.FlowChecker.VerifySpend(
tx,
Expand Down Expand Up @@ -131,6 +145,14 @@ func (e *StandardTxExecutor) ImportTx(tx *txs.ImportTx) error {
return err
}

var (
currentTimestamp = e.State.GetTimestamp()
isDurangoActive = e.Config.IsDurangoActivated(currentTimestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

e.Inputs = set.NewSet[ids.ID](len(tx.ImportedInputs))
utxoIDs := make([][]byte, len(tx.ImportedInputs))
for i, in := range tx.ImportedInputs {
Expand Down Expand Up @@ -209,6 +231,14 @@ func (e *StandardTxExecutor) ExportTx(tx *txs.ExportTx) error {
return err
}

var (
currentTimestamp = e.State.GetTimestamp()
isDurangoActive = e.Config.IsDurangoActivated(currentTimestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

outs := make([]*avax.TransferableOutput, len(tx.Outs)+len(tx.ExportedOutputs))
copy(outs, tx.Outs)
copy(outs[len(tx.Outs):], tx.ExportedOutputs)
Expand Down Expand Up @@ -386,6 +416,14 @@ func (e *StandardTxExecutor) TransformSubnetTx(tx *txs.TransformSubnetTx) error
return err
}

var (
currentTimestamp = e.State.GetTimestamp()
isDurangoActive = e.Config.IsDurangoActivated(currentTimestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
return err
}

// Note: math.MaxInt32 * time.Second < math.MaxInt64 - so this can never
// overflow.
if time.Duration(tx.MaxStakeDuration)*time.Second > e.Backend.Config.MaxStakeDuration {
Expand Down Expand Up @@ -512,6 +550,14 @@ func (e *StandardTxExecutor) BaseTx(tx *txs.BaseTx) error {
return err
}

var (
currentTimestamp = e.State.GetTimestamp()
isDurangoActive = e.Config.IsDurangoActivated(currentTimestamp)
)
if err := avax.VerifyMemoFieldLength(tx.Memo, isDurangoActive); err != nil {
StephenButtolph marked this conversation as resolved.
Show resolved Hide resolved
return err
}

// Verify the flowcheck
if err := e.FlowChecker.VerifySpend(
tx,
Expand Down
15 changes: 13 additions & 2 deletions vms/platformvm/txs/executor/standard_tx_executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,6 +1024,7 @@ func TestStandardTxExecutorDurangoAddValidator(t *testing.T) {
}

// Returns a RemoveSubnetValidatorTx that passes syntactic verification.
// Memo field is empty as required post Durango activation
func newRemoveSubnetValidatorTx(t *testing.T) (*txs.RemoveSubnetValidatorTx, *txs.Tx) {
t.Helper()

Expand Down Expand Up @@ -1066,7 +1067,6 @@ func newRemoveSubnetValidatorTx(t *testing.T) (*txs.RemoveSubnetValidatorTx, *tx
},
},
},
Memo: []byte("hi"),
},
},
Subnet: ids.GenerateTestID(),
Expand Down Expand Up @@ -1134,6 +1134,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
env := newValidRemoveSubnetValidatorTxVerifyEnv(t, ctrl)

// Set dependency expectations.
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetCurrentValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(env.staker, nil).Times(1)
subnetOwner := fx.NewMockOwner(ctrl)
env.state.EXPECT().GetSubnetOwner(env.unsignedTx.Subnet).Return(subnetOwner, nil).Times(1)
Expand Down Expand Up @@ -1196,6 +1197,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
newExecutor: func(ctrl *gomock.Controller) (*txs.RemoveSubnetValidatorTx, *StandardTxExecutor) {
env := newValidRemoveSubnetValidatorTxVerifyEnv(t, ctrl)
env.state = state.NewMockDiff(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetCurrentValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(nil, database.ErrNotFound)
env.state.EXPECT().GetPendingValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(nil, database.ErrNotFound)
e := &StandardTxExecutor{
Expand Down Expand Up @@ -1227,6 +1229,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
staker.Priority = txs.SubnetPermissionlessValidatorCurrentPriority

// Set dependency expectations.
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetCurrentValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(&staker, nil).Times(1)
e := &StandardTxExecutor{
Backend: &Backend{
Expand Down Expand Up @@ -1255,6 +1258,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
// Remove credentials
env.tx.Creds = nil
env.state = state.NewMockDiff(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetCurrentValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(env.staker, nil)
e := &StandardTxExecutor{
Backend: &Backend{
Expand All @@ -1281,6 +1285,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
newExecutor: func(ctrl *gomock.Controller) (*txs.RemoveSubnetValidatorTx, *StandardTxExecutor) {
env := newValidRemoveSubnetValidatorTxVerifyEnv(t, ctrl)
env.state = state.NewMockDiff(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetCurrentValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(env.staker, nil)
env.state.EXPECT().GetSubnetOwner(env.unsignedTx.Subnet).Return(nil, database.ErrNotFound)
e := &StandardTxExecutor{
Expand Down Expand Up @@ -1308,6 +1313,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
newExecutor: func(ctrl *gomock.Controller) (*txs.RemoveSubnetValidatorTx, *StandardTxExecutor) {
env := newValidRemoveSubnetValidatorTxVerifyEnv(t, ctrl)
env.state = state.NewMockDiff(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetCurrentValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(env.staker, nil)
subnetOwner := fx.NewMockOwner(ctrl)
env.state.EXPECT().GetSubnetOwner(env.unsignedTx.Subnet).Return(subnetOwner, nil)
Expand Down Expand Up @@ -1337,6 +1343,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
newExecutor: func(ctrl *gomock.Controller) (*txs.RemoveSubnetValidatorTx, *StandardTxExecutor) {
env := newValidRemoveSubnetValidatorTxVerifyEnv(t, ctrl)
env.state = state.NewMockDiff(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetCurrentValidator(env.unsignedTx.Subnet, env.unsignedTx.NodeID).Return(env.staker, nil)
subnetOwner := fx.NewMockOwner(ctrl)
env.state.EXPECT().GetSubnetOwner(env.unsignedTx.Subnet).Return(subnetOwner, nil)
Expand Down Expand Up @@ -1379,6 +1386,7 @@ func TestStandardExecutorRemoveSubnetValidatorTx(t *testing.T) {
}

// Returns a TransformSubnetTx that passes syntactic verification.
// Memo field is empty as required post Durango activation
func newTransformSubnetTx(t *testing.T) (*txs.TransformSubnetTx, *txs.Tx) {
t.Helper()

Expand Down Expand Up @@ -1421,7 +1429,6 @@ func newTransformSubnetTx(t *testing.T) (*txs.TransformSubnetTx, *txs.Tx) {
},
},
},
Memo: []byte("hi"),
},
},
Subnet: ids.GenerateTestID(),
Expand Down Expand Up @@ -1527,6 +1534,7 @@ func TestStandardExecutorTransformSubnetTx(t *testing.T) {
env := newValidTransformSubnetTxVerifyEnv(t, ctrl)
env.unsignedTx.MaxStakeDuration = math.MaxUint32
env.state = state.NewMockDiff(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
e := &StandardTxExecutor{
Backend: &Backend{
Config: &config.Config{
Expand Down Expand Up @@ -1554,6 +1562,7 @@ func TestStandardExecutorTransformSubnetTx(t *testing.T) {
// Remove credentials
env.tx.Creds = nil
env.state = state.NewMockDiff(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
e := &StandardTxExecutor{
Backend: &Backend{
Config: &config.Config{
Expand Down Expand Up @@ -1581,6 +1590,7 @@ func TestStandardExecutorTransformSubnetTx(t *testing.T) {
env := newValidTransformSubnetTxVerifyEnv(t, ctrl)
env.state = state.NewMockDiff(ctrl)
subnetOwner := fx.NewMockOwner(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetSubnetOwner(env.unsignedTx.Subnet).Return(subnetOwner, nil)
env.state.EXPECT().GetSubnetTransformation(env.unsignedTx.Subnet).Return(nil, database.ErrNotFound).Times(1)
env.fx.EXPECT().VerifyPermission(gomock.Any(), env.unsignedTx.SubnetAuth, env.tx.Creds[len(env.tx.Creds)-1], subnetOwner).Return(nil)
Expand Down Expand Up @@ -1615,6 +1625,7 @@ func TestStandardExecutorTransformSubnetTx(t *testing.T) {

// Set dependency expectations.
subnetOwner := fx.NewMockOwner(ctrl)
env.state.EXPECT().GetTimestamp().Return(env.latestForkTime)
env.state.EXPECT().GetSubnetOwner(env.unsignedTx.Subnet).Return(subnetOwner, nil).Times(1)
env.state.EXPECT().GetSubnetTransformation(env.unsignedTx.Subnet).Return(nil, database.ErrNotFound).Times(1)
env.fx.EXPECT().VerifyPermission(env.unsignedTx, env.unsignedTx.SubnetAuth, env.tx.Creds[len(env.tx.Creds)-1], subnetOwner).Return(nil).Times(1)
Expand Down
2 changes: 2 additions & 0 deletions vms/platformvm/txs/unsigned_tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ type UnsignedTx interface {
Outputs() []*avax.TransferableOutput

// Attempts to verify this transaction without any provided state.
// Note that some other syntactic checks may depend on state so they
// are not carried out in this method
abi87 marked this conversation as resolved.
Show resolved Hide resolved
SyntacticVerify(ctx *snow.Context) error

// Visit calls [visitor] with this transaction's concrete type
Expand Down
Loading