Skip to content

chore: document BurnchainConfig parameters #6005

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Jiloc
Copy link
Contributor

@Jiloc Jiloc commented Apr 13, 2025

Description

Add documentation for all BurnchainConfig parameters

Applicable issues

second step of #5898

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@Jiloc Jiloc requested a review from a team as a code owner April 13, 2025 16:29
@Jiloc Jiloc changed the title add BurnchainConfig documentation chore: document BurnchainConfig parameters Apr 13, 2025
@Jiloc Jiloc self-assigned this Apr 13, 2025
@Jiloc Jiloc requested review from obycode and wileyj April 13, 2025 17:46
/// This setting determines network parameters (like chain ID, peer version),
/// default configurations, genesis block definitions, and overall node behavior.
///
/// Supported values: `"mainnet"`, `"mocknet"`, `"helium"`, `"neon"`, `"argon"`, `"krypton"`, `"xenon"`, `"nakamoto-neon"`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'd argue to format this in a table (at least for now) because one of the questions that comes up is "what is xenon?"
perhaps something like this:

Supported values:
    mainnet => mainnet
    xenon => testnet
    mocknet => regtest
    ...

i recognize it may be a bit messy - just trying to think of a way to make it more clear for users later on

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 like the idea, i can make a table

Comment on lines 1226 to 1229
/// The network-specific identifier used in P2P communication and database initialization .
/// Derived from `mode` during config load, unless explicitly overridden (for test purposes).
///
/// Default: Derived from [`BurnchainConfig::mode`] ([`CHAIN_ID_MAINNET`] for `mainnet`, [`CHAIN_ID_TESTNET`] otherwise).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should add a warning to several of these settings i.e. "Don't modify this unless you really know what you're doing"
chain_id is a great example of this.

Comment on lines 1245 to 1253
/// The Maximum amount (in sats) of "burn commitment" to broadcast for the next block's leader election.
/// Acts as a safety cap to limit the maximum amount spent on mining.
///
/// It serves as both the target fee and a fallback if dynamic fee calculations fail or cannot be performed
///
/// This setting can be hot-reloaded from a miner's config file, allowing adjustment without restarting.
/// Primarily relevant for miners.
///
/// Default: `20000` satoshis
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably have a disclaimer that it's only used when mining

Copy link
Contributor

Choose a reason for hiding this comment

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

Might make sense to start an issue that this could be moved to the miner config section.

pub username: Option<String>,
/// The password for authenticating with the bitcoin node's RPC interface.
/// Required if the bitcoin node requires RPC authentication.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems inconsistent, otherwise we would want to add Required if... to several other settings in this PR.
i.e.

Required if bitcoin p2p is on a non-default port

I'm not convinced of either option, it just seems a bit inconsistent is all.

/// These two-byte identifiers help ensure that nodes only connect to peers on the same network type.
/// Common values include:
/// - "X2" for mainnet
/// - "T2" for Xenon testnet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// - "T2" for Xenon testnet
/// - "T2" for testnet (xenon)

/// - The number of defined epochs cannot exceed the maximum supported by the node software.
///
/// Applied only if [`BurnchainConfig::mode`] is not "mainnet".
/// This is intended strictly for testing purposes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inconsistent use of this line. for example, pox_reward_length and pox_2_activation should probably also have this line (as well as a few others in this PR).
I think it should be present on any setting that is used for testing only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants