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

Jump to bottom

refactor: version constants for msg limits #3982

Closed
wants to merge 2 commits into from
Closed

refactor: version constants for msg limits #3982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e476c3
refactor: version constants for msg limits
rootulp Oct 16, 2024
00bef1e
Merge branch 'main' into rp/version-constants-limit-tx
rootulp Oct 16, 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
24 changes: 12 additions & 12 deletions app/test/prepare_proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ func TestPrepareProposalCappingNumberOfMessages(t *testing.T) {
signers = append(signers, signer)
}

numberOfPFBs := appconsts.PFBTransactionCap + 500
numberOfPFBs := appconsts.PFBTransactionCap(testApp.AppVersion()) + 500
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider caching testApp.AppVersion() for improved readability

The function testApp.AppVersion() is called multiple times throughout the test. To enhance readability and avoid redundant function calls, consider storing the application version in a local variable:

appVersion := testApp.AppVersion()

Then, replace subsequent calls with appVersion.

Also applies to: 336-336, 357-358, 362-363, 367-368, 379-380, 394-396

pfbTxs := make([][]byte, 0, numberOfPFBs)
randomBytes := make([]byte, 2000)
_, err := rand.Read(randomBytes)
Expand Down Expand Up @@ -333,7 +333,7 @@ func TestPrepareProposalCappingNumberOfMessages(t *testing.T) {
accountIndex++
}

numberOfMsgSends := appconsts.NonPFBTransactionCap + 500
numberOfMsgSends := appconsts.NonPFBTransactionCap(testApp.AppVersion()) + 500
msgSendTxs := make([][]byte, 0, numberOfMsgSends)
for i := 0; i < numberOfMsgSends; i++ {
msg := banktypes.NewMsgSend(
Expand All @@ -354,18 +354,18 @@ func TestPrepareProposalCappingNumberOfMessages(t *testing.T) {
}{
{
name: "capping only PFB transactions",
inputTransactions: pfbTxs[:appconsts.PFBTransactionCap+50],
expectedTransactions: pfbTxs[:appconsts.PFBTransactionCap],
inputTransactions: pfbTxs[:appconsts.PFBTransactionCap(testApp.AppVersion())+50],
expectedTransactions: pfbTxs[:appconsts.PFBTransactionCap(testApp.AppVersion())],
},
{
name: "capping only PFB transactions with multiple messages",
inputTransactions: multiPFBsPerTxs[:appconsts.PFBTransactionCap],
expectedTransactions: multiPFBsPerTxs[:appconsts.PFBTransactionCap/numberOfMsgsPerTx],
inputTransactions: multiPFBsPerTxs[:appconsts.PFBTransactionCap(testApp.AppVersion())],
expectedTransactions: multiPFBsPerTxs[:appconsts.PFBTransactionCap(testApp.AppVersion())/numberOfMsgsPerTx],
},
{
name: "capping only msg send transactions",
inputTransactions: msgSendTxs[:appconsts.NonPFBTransactionCap+50],
expectedTransactions: msgSendTxs[:appconsts.NonPFBTransactionCap],
inputTransactions: msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion())+50],
expectedTransactions: msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion())],
},
{
name: "capping msg send after pfb transactions",
Expand All @@ -376,8 +376,8 @@ func TestPrepareProposalCappingNumberOfMessages(t *testing.T) {
return input
}(),
expectedTransactions: func() [][]byte {
expected := make([][]byte, 0, appconsts.NonPFBTransactionCap+100)
expected = append(expected, msgSendTxs[:appconsts.NonPFBTransactionCap]...)
expected := make([][]byte, 0, appconsts.NonPFBTransactionCap(testApp.AppVersion())+100)
expected = append(expected, msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion())]...)
Comment on lines +379 to +380
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Adjust the capacity of the expected slice for accuracy

In lines 379 and 394, the capacity of the expected slice includes an extra +100:

expected := make([][]byte, 0, appconsts.NonPFBTransactionCap(testApp.AppVersion()) + 100)
expected := make([][]byte, 0, appconsts.PFBTransactionCap(testApp.AppVersion()) + 100)

Since you're appending only up to the transaction cap, the additional capacity may be unnecessary. Consider adjusting the capacity to match the exact number of expected transactions for clarity and to avoid potential misunderstandings.

Also applies to: 394-396

expected = append(expected, pfbTxs[:100]...)
return expected
}(),
Expand All @@ -391,9 +391,9 @@ func TestPrepareProposalCappingNumberOfMessages(t *testing.T) {
return input
}(),
expectedTransactions: func() [][]byte {
expected := make([][]byte, 0, appconsts.PFBTransactionCap+100)
expected := make([][]byte, 0, appconsts.PFBTransactionCap(testApp.AppVersion())+100)
expected = append(expected, msgSendTxs[:100]...)
expected = append(expected, pfbTxs[:appconsts.PFBTransactionCap]...)
expected = append(expected, pfbTxs[:appconsts.PFBTransactionCap(testApp.AppVersion())]...)
return expected
}(),
},
Expand Down
4 changes: 2 additions & 2 deletions app/validate_txs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler
ctx = ctx.WithTxBytes(tx)

msgTypes := msgTypes(sdkTx)
if nonPFBTransactionsCount+len(sdkTx.GetMsgs()) > appconsts.NonPFBTransactionCap {
if nonPFBTransactionsCount+len(sdkTx.GetMsgs()) > appconsts.NonPFBTransactionCap(ctx.ConsensusParams().Version.AppVersion) {
logger.Debug("skipping tx because the sdk message cap was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()))
continue
}
Expand Down Expand Up @@ -101,7 +101,7 @@ func filterBlobTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handle
// Set the tx size on the context before calling the AnteHandler
ctx = ctx.WithTxBytes(tx.Tx)

if pfbTransactionCount+len(sdkTx.GetMsgs()) > appconsts.PFBTransactionCap {
if pfbTransactionCount+len(sdkTx.GetMsgs()) > appconsts.PFBTransactionCap(ctx.ConsensusParams().Version.AppVersion) {
logger.Debug("skipping tx because the pfb transaction cap was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash()))
continue
}
Expand Down
9 changes: 0 additions & 9 deletions pkg/appconsts/global_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,3 @@ func UpgradeHeightDelay() int64 {
func HashLength() int {
return hashLength
}

// The following consts are not consensus breaking and will be applied straight after this binary is started.
const (
// NonPFBTransactionCap is the maximum number of SDK messages, aside from PFBs, that a block can contain.
NonPFBTransactionCap = 200

// PFBTransactionCap is the maximum number of PFB messages a block can contain.
PFBTransactionCap = 600
)
6 changes: 6 additions & 0 deletions pkg/appconsts/v2/app_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,10 @@ const (
Version uint64 = 2
SquareSizeUpperBound int = 128
SubtreeRootThreshold int = 64
// NonPFBTransactionCap is the maximum number of SDK messages, aside from
// PFBs, that a block can contain.
NonPFBTransactionCap = 200
// PFBTransactionCap is the maximum number of PFB messages a block can
// contain.
PFBTransactionCap = 600
)
6 changes: 6 additions & 0 deletions pkg/appconsts/v3/app_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,10 @@ const (
TxSizeCostPerByte uint64 = 10
GasPerBlobByte uint32 = 8
MaxTxSize int = 2097152 // 2 MiB in bytes
// NonPFBTransactionCap is the maximum number of SDK messages, aside from
// PFBs, that a block can contain.
NonPFBTransactionCap = 200
// PFBTransactionCap is the maximum number of PFB messages a block can
// contain.
PFBTransactionCap = 600
)
8 changes: 8 additions & 0 deletions pkg/appconsts/versioned_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ func MaxTxSize(_ uint64) int {
return v3.MaxTxSize
}

func NonPFBTransactionCap(_ uint64) int {
return v3.NonPFBTransactionCap
}

func PFBTransactionCap(_ uint64) int {
return v3.PFBTransactionCap
}

var (
DefaultSubtreeRootThreshold = SubtreeRootThreshold(LatestVersion)
DefaultSquareSizeUpperBound = SquareSizeUpperBound(LatestVersion)
Expand Down
24 changes: 24 additions & 0 deletions pkg/appconsts/versioned_consts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,30 @@ func TestVersionedConsts(t *testing.T) {
expectedConstant: v3.MaxTxSize,
got: appconsts.MaxTxSize(v3.Version),
},
{
Copy link
Member

Choose a reason for hiding this comment

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

doesn't have to be addressed in this pr but we probably need to come up with a more efficient way of testing this before it gets out of hand with all the versions

name: "NonPFBTransactionCap v2",
version: v2.Version,
expectedConstant: v2.NonPFBTransactionCap,
got: appconsts.NonPFBTransactionCap(v2.Version),
},
{
name: "NonPFBTransactionCap v3",
version: v3.Version,
expectedConstant: v3.NonPFBTransactionCap,
got: appconsts.NonPFBTransactionCap(v3.Version),
},
{
name: "PFBTransactionCap v2",
version: v2.Version,
expectedConstant: v2.PFBTransactionCap,
got: appconsts.PFBTransactionCap(v2.Version),
},
{
name: "PFBTransactionCap v3",
version: v3.Version,
expectedConstant: v3.PFBTransactionCap,
got: appconsts.PFBTransactionCap(v3.Version),
},
}

for _, tc := range testCases {
Expand Down
Loading