doc: Remove stale sections in dev notes

maflcko commented at 4:51 pm on May 20, 2025: member

This removes sections that I’ve been collecting as stale or overly redundant over the years. The rationale for each removal is in the commit message.

doc: Remove -disablewallet from dev notes

No setting should crash. Most settings are tested as part of the
functional test suite, including -disablewallet.

fa69c5b170

DrahtBot commented at 4:51 pm on May 20, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32572.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	1440000bytes

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Docs on May 20, 2025

maflcko force-pushed on May 20, 2025

1440000bytes commented at 6:06 pm on May 20, 2025: none

ACK https://github.com/bitcoin/bitcoin/pull/32572/commits/fa2482adf5d2df2192e882dc9ca817056ea52087

in doc/developer-notes.md:1009 in fa92413bc9 outdated

1003@@ -1004,10 +1004,6 @@ Strings and formatting
1004     buffer overflows, and surprises with `\0` characters. Also, some C string manipulations
1005     tend to act differently depending on platform, or even the user locale.
1006 
1007-- For `strprintf`, `LogInfo`, `LogDebug`, etc formatting characters don't need size specifiers.
1008-
1009-  - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion.

davidgumberg commented at 6:55 pm on May 20, 2025:

Although, the phrase “size specifiers” is used here which I believe is incorrect, the spirit of this remark is still helpful.

Using the terminology from this article: https://en.cppreference.com/w/c/io/fprintf I believe the more correct way to word the note is, “- For strprintf, LogInfo, LogDebug, etc: When using conversion specifiers like %d, don’t use length modifiers (e.g.: %ld).

maflcko commented at 7:02 pm on May 20, 2025:

I see, but I don’t think it matters. If it mattered, we should remove the existing length modifiers in the code and enforce this rule at compile time (ConstevalFormatString). If it doesn’t matter, there is no need to document it.

laanwj commented at 11:48 am on May 21, 2025:

The reason i added this back in the day is that 64-bit types are awful to handle in a cross-platform way in printf family functions. This necessitated use of macros like PRIx64, which didn’t even work on windows (or something) and actually actively break with tinyformat. So just being able to use %d and %x is very nice and saves thinking about this.

No idea what would be a better place to document this, maybe it’s so obvious (to us) now that it doesn’t warrant mentioning at all anymore, i don’t know.

Sjors commented at 12:09 pm on May 21, 2025:

It’s not obvious (to me), but as long as the compiler tells me what to do… otherwise a note is helpful.

laanwj commented at 2:00 pm on May 21, 2025:

Well the idea was that by the time the compiler gets to it, a developer may already have done a frustrating search “what % expression do i need to use”. Documenting it could avoid that (i don’t think this particular blurb is “stale” in any way besides “not changed in a long time”).

That said, there are tons of examples of strprintf in the code, and nothing uses length modifiers. Tinyformat also uses %s for strings without c_str(), which isn’t explicitly documented.

Edit: okay actually there are a few but it’s very rare, i guess all holdovers from satoshi’s code:

0src/net_processing.cpp:            LogDebug(BCLog::NET, "getblocks locator size %lld > %d, %s\n", locator.vHave.size(), MAX_LOCATOR_SZ, pfrom.DisconnectMsg(fLogIPs));
1src/net_processing.cpp:            LogDebug(BCLog::NET, "getheaders locator size %lld > %d, %s\n", locator.vHave.size(), MAX_LOCATOR_SZ, pfrom.DisconnectMsg(fLogIPs));
2src/node/miner.cpp:    LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost);
3src/policy/fees.cpp:        LogPrintf("Fee estimation file %s too old (age=%lld > %lld hours) and will not be used to avoid serving stale estimates.\n", fs::PathToString(m_estimation_filepath), Ticks<std::chrono::hours>(file_age), Ticks<std::chrono::hours>(MAX_FILE_AGE));
4src/validation.cpp:            log_string += strprintf("New package %s with %lu txs, fees=%s, vsize=%s",
5src/validation.cpp:    LogPrintf("%s%s: new best=%s height=%d version=0x%08x log2_work=%f tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)%s\n",
6src/wallet/spend.cpp:            wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n",

maflcko commented at 2:23 pm on May 21, 2025:

Tinyformat also uses %s for strings without c_str(), which isn’t explicitly documented.

It is documented in this file ;)

Well the idea was that by the time the compiler gets to it, a developer may already have done a frustrating search “what % expression do i need to use”. Documenting it could avoid that

Ok, restored and clarified it for now. It will probably go away in 3 years with C++23 std::format

in doc/developer-notes.md:819 in fae5badfdf outdated

814-These are not committed but apply only to one repository.
815-
816-If a set of tools is used by the build system or scripts the repository (for
817-example, lcov) it is perfectly acceptable to add its files to `.gitignore`
818-and commit them.
819-

davidgumberg commented at 6:59 pm on May 20, 2025:

I slightly disagree with removing this: I personally found this note helpful pre cmake migration, when the recommended way to generate compile_commands.json was with bear, and Bitcoin Core’s .gitignore didn’t exclude this.

maflcko commented at 7:02 pm on May 20, 2025:

This was added to the gitignore file itself in commit fada115cbeaa8c0ca3d19178499079b66ee5f499. The past violations are evidence that the note in the dev notes was missed anyway.

I’ve listed some in #32417#issue-3038116748

If there is a file-specific note, it seems easier to just have it in the file itself.

Sjors commented at 10:11 am on May 21, 2025:

.gitignore is indeed a great place to document this, as it’s the file anyone will attempt to edit when their IDE leaves an artefact behind.

in doc/developer-notes.md:1163 in fa220ec193 outdated

1115@@ -1116,31 +1116,6 @@ TRY_LOCK(cs_vNodes, lockNodes);
1116 }
1117 ```
1118 
1119-Scripts
1120---------------------------
1121-
1122-Write scripts in Python rather than bash, when possible.

Sjors commented at 10:12 am on May 21, 2025:

fa220ec193436fd1e62e29a8f3525bf26fa3c209: this is still useful, even if often ignored :-)

maflcko commented at 10:31 am on May 21, 2025:

often ignored :-)

I don’t see any bash scripts. Apart from pre-existing scripts (ci and contrib stuff).

this is still useful

It should probably say Rust, though? :sweat_smile:

Sjors commented at 12:09 pm on May 21, 2025:

Not to bash rust, but maybe suggest either…

laanwj commented at 12:19 pm on May 21, 2025:

“Don’t add any new bash scripts” would still be good advice imo. We’re still running into bash insanity a decade later #32573. No opinion on rust versus python they’re both 100% better.

maflcko commented at 2:24 pm on May 21, 2025:

maybe suggest either…

thx, done

yancyribbens commented at 0:07 am on May 22, 2025:

Write scripts in Python or Rust rather than bash, when possible.

technically only Python is a “scripting language” for writing “scripts”. I’m not sure you could say programs written in Rust are scripts, although I’m sure people will know what is intended by this sentence.

maflcko commented at 1:55 pm on May 22, 2025:

programs

thx, I may address this, if I have to re-touch for other reasons

Sjors commented at 10:17 am on May 21, 2025: member

I see the benefit of not duplicating linter code in developer notes. However it’s quite annoying to find out about linter failures only after pushing.

The developer notes could point to the incantation to run all the linters in a container.

Otherwise looks good.

doc: Remove note about removed ParsePrechecks faf2094f25

doc: Remove .gitignore section

This was added to the gitignore file itself in commit
fada115cbeaa8c0ca3d19178499079b66ee5f499. The past violations are
evidence that the note in the dev notes was missed anyway.

faf65f0531

doc: Remove shebang section

This is already checked by test/lint/lint-files.py

There is no need to reword all linters into the dev notes.

Also, allow scripts in Rust (there are already some).

7777fb8bc7

doc: Remove file name section

This is already checked by test/lint/lint-files.py

There is no need to reword all linters into the dev notes.

fa6623d85a

doc: Remove dev note section on includes fad6cd739b

doc: Remove section about include guards fa00b8c02c

doc: Remove section about RPC arg names in table

The table no longer holds arg names. This is done automatically by
RPCHelpMan.

2222d61e1c

doc: Remove section about RPC alias via function pointer

This is no longer possible with RPCHelpMan.

faaf34ad72

doc: Clarify strprintf size specifier note fac8b05197

doc: Clarify and move "hygienic commit" note

The prior version mentions to update the tests *first*, which is wrong.
It must be updated at the same time in the same commit.

fab79c1a25

doc: Move CI-must-pass requirement into readme section

Seems fine to state it clearly once.

fac00d4ed3

in CONTRIBUTING.md:1 in fa2482adf5

janb84 commented at 11:28 am on May 21, 2025:

Small suggestion also add in CONTRIBUTING to wait for CI to pass. This creates a nice sort of checklist and is inline with the suggested changes to remove the statement from developer-notes and add it to the readme.

Lines 136-137:

Push changes to your fork
Wait for the CI to pass
Create pull request

maflcko commented at 2:29 pm on May 21, 2025:

Small suggestion also add in CONTRIBUTING to wait for CI to pass.

Lines 136-137:
* Push changes to your fork

* Wait for the CI to pass

* Create pull request

I don’t think this works. Contributors may have to enable GHA, which they may not want. Also, the Cirrus tasks are tedious to setup and require hardware/vm runners. Also, some tasks do different things in a pull request vs branch. Finally, it seems overkill to run (and wait for) CI twice.

Most of the time it is sufficient to just compile the code (and just run the corresponding test, if there is one).

If there is an unexpected CI error for whatever reason, it is fine to debug it locally and address it the next day (or week).

maflcko force-pushed on May 21, 2025

maflcko commented at 2:31 pm on May 21, 2025: member

I see the benefit of not duplicating linter code in developer notes. However it’s quite annoying to find out about linter failures only after pushing.

The developer notes could point to the incantation to run all the linters in a container.

The lint CI should always run and complete within 60 seconds after pushing. If not, please let me know.

Your note makes sense. I’ll consider it in the next doc update, which moves stuff around.

doc: Remove stale sections in dev notes #32572

Code Coverage & Benchmarks

Reviews