Skip to content

Conversation

@jannotti
Copy link
Contributor

@jannotti jannotti commented May 12, 2025

This is a first step to remove the use of the config package in the basics package.

We use many constants, derived from ConsensusParams, or checked against ConsensusParams, as allocbounds. Therefore, types in basics end up needing to use the config package to access them, so the config package can't use basics which prevents a lot of natural typing choices.

This PR moves all such variables and constants to a bounds package. I think this also helps clearly separate them from the consensus parameters themselves, and they now appear as allocbound=bounds.Max.... which seems clearer.

Summary

Test Plan

It was not used anywhere. Presumably, it was written thinking that a
StateDelta would be check after making it. But instead, each step of
creating the StateDelta is checked.
@jannotti jannotti self-assigned this May 12, 2025
@jannotti jannotti force-pushed the move-allocbounds branch from a52c8c8 to 01aa06a Compare May 12, 2025 18:45
@jannotti jannotti marked this pull request as draft May 12, 2025 18:47
@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes missing coverage. Please review.

Project coverage is 51.81%. Comparing base (18990e0) to head (8b15db9).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
daemon/algod/api/server/v2/handlers.go 0.00% 1 Missing ⚠️
tools/debug/transplanter/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6324      +/-   ##
==========================================
- Coverage   51.82%   51.81%   -0.01%     
==========================================
  Files         652      652              
  Lines       87425    87425              
==========================================
- Hits        45307    45300       -7     
- Misses      39249    39255       +6     
- Partials     2869     2870       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jannotti jannotti force-pushed the move-allocbounds branch from 01aa06a to 1e2a1d5 Compare May 12, 2025 19:15
By moving bounds into their own package, it is both
clearer (config.MaxTxGroupSize was never a _config_ value, really, it
was a derived bound, based on config values.) and allows the `basic`
to import only these limits, with importing `config`.  In turn, that
will mean that we can import `basic` into `config` so
`ConsensusParams` can have types in it like `basics.Round`.
@jannotti jannotti force-pushed the move-allocbounds branch from 1e2a1d5 to b92a804 Compare May 12, 2025 20:47
@jannotti jannotti marked this pull request as ready for review May 15, 2025 02:14
@jannotti jannotti force-pushed the move-allocbounds branch from e5f7f39 to 0b3a4db Compare May 16, 2025 15:53
@jannotti jannotti force-pushed the move-allocbounds branch from 0b3a4db to 8b15db9 Compare May 16, 2025 16:00
@jannotti jannotti requested review from algorandskiy and gmalouf May 16, 2025 16:25
@cce cce requested a review from Copilot May 16, 2025 17:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR decouples allocation bound constants from the config package by moving them into a new bounds package, clarifying their role and simplifying dependency relationships between packages. Key changes include:

  • Updating import paths and references from config to config/bounds across multiple packages (transactions, bookkeeping, basics, appRateLimiter, etc.).
  • Adjusting codec struct tags and size calculations in msgp generation functions to reference the new bounds constants.
  • Updating tests and consensus parameter checks to work with the new bounds package.

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated no comments.

Show a summary per file
File Description
data/transactions/logic/eval_test.go Updated import and require check for MaxTxGroupSize
data/transactions/logic/eval.go Replaced config constants with bounds in constant definitions
data/transactions/application.go Updated codec struct tags for application arguments and programs
data/bookkeeping/msgp_gen.go Switched from config to bounds in error checks for genesis and log sizes
data/bookkeeping/block.go Updated struct tags to use bounds constants for GenesisID and Payset
data/basics/userBalance_test.go Removed an allocation bounds test referencing config constants
data/basics/userBalance.go Updated codec tags on maps and programs to use bounds constants
data/basics/teal.go Adjusted codec tags to reference bounds for TEAL key/value sizes
data/basics/msgp_gen.go Replaced references from config to bounds in decoding overflow checks
data/appRateLimiter_test.go & data/appRateLimiter.go Updated TxGroup size and pool pre-allocation constants to use bounds
daemon/algod/api/server/v2/handlers.go Updated decodeTxGroup call to use bounds.MaxTxGroupSize
crypto/stateproof/weights_test.go Updated tests to use bounds constants for state proof computations
crypto/stateproof/structs.go Updated inline documentation to reference bounds constants
config/consensus.go Updated bounds checks to use the new bounds package
config/config_test.go Adjusted tests referencing allocation bounds to use bounds constants
config/config.go No functional change; remains lower in priority after bounds migration
config/bounds/bounds.go Introduced new package with all bound declarations and constants
agreement/msgp_gen.go & agreement/bundle.go Updated codec tags and overflow checks to reference bounds constants

@jannotti jannotti merged commit ad67b95 into algorand:master May 16, 2025
19 checks passed
cce pushed a commit to cce/go-algorand that referenced this pull request May 29, 2025
@jannotti jannotti deleted the move-allocbounds branch July 15, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants