From fa69c5b170f56d554fcb0d0887bd27f961fe3e74 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 17:35:10 +0200 Subject: [PATCH 01/12] doc: Remove -disablewallet from dev notes No setting should crash. Most settings are tested as part of the functional test suite, including -disablewallet. --- doc/developer-notes.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 4759146a8fc..e577da05b63 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -879,11 +879,6 @@ Note that the format strings and parameters of `LogDebug` and `LogTrace` are only evaluated if the logging category is enabled, so you must be careful to avoid side-effects in those expressions. -Wallet -------- - -- Make sure that no crashes happen with run-time option `-disablewallet`. - General C++ ------------- From faf2094f2511a5322d68d2352f244b5bc89d5fee Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 17:42:26 +0200 Subject: [PATCH 02/12] doc: Remove note about removed ParsePrechecks --- doc/developer-notes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index e577da05b63..18d8388a7ea 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1028,7 +1028,7 @@ Strings and formatting - In cases where you do call `.c_str()`, you might want to additionally check that the string does not contain embedded '\0' characters, because it will (necessarily) truncate the string. This might be used to hide parts of the string from logging or to circumvent checks. If a use of strings is sensitive to this, take care to check the string for embedded NULL characters first - and reject it if there are any (see `ParsePrechecks` in `strencodings.cpp` for an example). + and reject it if there are any. Shadowing -------------- From faf65f05312be7647f485f088ba00fef97f47bf4 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 17:59:42 +0200 Subject: [PATCH 03/12] 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. --- doc/developer-notes.md | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 18d8388a7ea..7516f86e84b 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -785,38 +785,6 @@ Threads - [ThreadI2PAcceptIncoming (`b-i2paccept`)](https://doxygen.bitcoincore.org/class_c_connman.html#a57787b4f9ac847d24065fbb0dd6e70f8) : Listens for and accepts incoming I2P connections through the I2P SAM proxy. -Ignoring IDE/editor files --------------------------- - -In closed-source environments in which everyone uses the same IDE, it is common -to add temporary files it produces to the project-wide `.gitignore` file. - -However, in open source software such as Bitcoin Core, where everyone uses -their own editors/IDE/tools, it is less common. Only you know what files your -editor produces and this may change from version to version. The canonical way -to do this is thus to create your local gitignore. Add this to `~/.gitconfig`: - -``` -[core] - excludesfile = /home/.../.gitignore_global -``` - -(alternatively, type the command `git config --global core.excludesfile ~/.gitignore_global` -on a terminal) - -Then put your favourite tool's temporary filenames in that file, e.g. -``` -# NetBeans -nbproject/ -``` - -Another option is to create a per-repository excludes file `.git/info/exclude`. -These are not committed but apply only to one repository. - -If a set of tools is used by the build system or scripts the repository (for -example, lcov) it is perfectly acceptable to add its files to `.gitignore` -and commit them. - Development guidelines ============================ From 7777fb8bc749e18c178ef460b65219187e676128 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 18:06:44 +0200 Subject: [PATCH 04/12] 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). --- doc/developer-notes.md | 22 +--------------------- test/lint/lint-files.py | 3 +++ 2 files changed, 4 insertions(+), 21 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 7516f86e84b..5c99a7a33e7 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1123,27 +1123,7 @@ TRY_LOCK(cs_vNodes, lockNodes); Scripts -------------------------- -Write scripts in Python rather than bash, when possible. - -### Shebang - -- Use `#!/usr/bin/env bash` instead of obsolete `#!/bin/bash`. - - - [*Rationale*](https://github.com/dylanaraps/pure-bash-bible#shebang): - - `#!/bin/bash` assumes it is always installed to /bin/ which can cause issues; - - `#!/usr/bin/env bash` searches the user's PATH to find the bash binary. - - OK: -```bash -#!/usr/bin/env bash -``` - - Wrong: -```bash -#!/bin/bash -``` +Write scripts in Python or Rust rather than bash, when possible. Source code organization -------------------------- diff --git a/test/lint/lint-files.py b/test/lint/lint-files.py index 681785acd23..c5af959e0a8 100755 --- a/test/lint/lint-files.py +++ b/test/lint/lint-files.py @@ -26,6 +26,9 @@ ALLOWED_SOURCE_FILENAME_EXCEPTION_REGEXP = ( ALLOWED_PERMISSION_NON_EXECUTABLES = 0o644 ALLOWED_PERMISSION_EXECUTABLES = 0o755 ALLOWED_EXECUTABLE_SHEBANG = { + # https://github.com/dylanaraps/pure-bash-bible#shebang: + # `#!/bin/bash` assumes it is always installed to /bin/ which can cause issues; + # `#!/usr/bin/env bash` searches the user's PATH to find the bash binary. "py": [b"#!/usr/bin/env python3"], "sh": [b"#!/usr/bin/env bash", b"#!/bin/sh"], } From fa6623d85af192c8aa1d0380d4a5452e0779c64e Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 18:11:55 +0200 Subject: [PATCH 05/12] 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. --- doc/developer-notes.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 5c99a7a33e7..ff6300cc08c 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1133,12 +1133,6 @@ Source code organization - *Rationale*: Shorter and simpler header files are easier to read and reduce compile time. -- Use only the lowercase alphanumerics (`a-z0-9`), underscore (`_`) and hyphen (`-`) in source code filenames. - - - *Rationale*: `grep`:ing and auto-completing filenames is easier when using a consistent - naming pattern. Potential problems when building on case-insensitive filesystems are - avoided when using only lowercase characters in source code filenames. - - Every `.cpp` and `.h` file should `#include` every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers. From fad6cd739b638cf7e8c86cdccf98df070e64a0f9 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 18:27:22 +0200 Subject: [PATCH 06/12] doc: Remove dev note section on includes --- doc/developer-notes.md | 8 -------- test/lint/lint-includes.py | 4 ++++ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index ff6300cc08c..b9965b4f4d3 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1160,14 +1160,6 @@ namespace { - *Rationale*: Avoids confusion about the namespace context. -- Use `#include ` bracket syntax instead of - `#include "primitives/transactions.h"` quote syntax. - - - *Rationale*: Bracket syntax is less ambiguous because the preprocessor - searches a fixed list of include directories without taking location of the - source file into account. This allows quoted includes to stand out more when - the location of the source file actually is relevant. - - Use include guards to avoid the problem of double inclusion. The header file `foo/bar.h` should use the include guard identifier `BITCOIN_FOO_BAR_H`, e.g. diff --git a/test/lint/lint-includes.py b/test/lint/lint-includes.py index 05780c30746..b0c91b90114 100755 --- a/test/lint/lint-includes.py +++ b/test/lint/lint-includes.py @@ -166,6 +166,10 @@ def main(): # Enforce bracket syntax includes quote_syntax_inclusions = find_quote_syntax_inclusions() + # *Rationale*: Bracket syntax is less ambiguous because the preprocessor + # searches a fixed list of include directories without taking location of the + # source file into account. This allows quoted includes to stand out more when + # the location of the source file actually is relevant. if quote_syntax_inclusions: print("Please use bracket syntax includes (\"#include \") instead of quote syntax includes:") From fa00b8c02c9de6dca833b83dc6447ba31d72ca57 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 18:20:32 +0200 Subject: [PATCH 07/12] doc: Remove section about include guards --- doc/developer-notes.md | 10 ---------- test/lint/lint-include-guards.py | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index b9965b4f4d3..ed2fa64b591 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1160,16 +1160,6 @@ namespace { - *Rationale*: Avoids confusion about the namespace context. -- Use include guards to avoid the problem of double inclusion. The header file - `foo/bar.h` should use the include guard identifier `BITCOIN_FOO_BAR_H`, e.g. - -```c++ -#ifndef BITCOIN_FOO_BAR_H -#define BITCOIN_FOO_BAR_H -... -#endif // BITCOIN_FOO_BAR_H -``` - GUI ----- diff --git a/test/lint/lint-include-guards.py b/test/lint/lint-include-guards.py index 77af05c1c2e..e16bb94a770 100755 --- a/test/lint/lint-include-guards.py +++ b/test/lint/lint-include-guards.py @@ -83,7 +83,7 @@ def main(): if count != 3: print(f'{header_file} seems to be missing the expected ' - 'include guard:') + 'include guard to prevent the double inclusion problem:') print(f' #ifndef {header_id}') print(f' #define {header_id}') print(' ...') From 2222d61e1ce5c9efd099f33b5dda934bc9d2d57f Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 18:28:32 +0200 Subject: [PATCH 08/12] doc: Remove section about RPC arg names in table The table no longer holds arg names. This is done automatically by RPCHelpMan. --- doc/developer-notes.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index ed2fa64b591..cebc34d1cc8 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1398,10 +1398,6 @@ A few guidelines for introducing and reviewing new RPC interfaces: - *Rationale*: Integer verbosity allows for multiple values. Undocumented boolean verbosity is deprecated and new RPC methods should prevent its use. -- Don't forget to fill in the argument names correctly in the RPC command table. - - - *Rationale*: If not, the call cannot be used with name-based arguments. - - Add every non-string RPC argument `(method, idx, name)` to the table `vRPCConvertParams` in `rpc/client.cpp`. - *Rationale*: `bitcoin-cli` and the GUI debug console use this table to determine how to From faaf34ad7253e3d347af986305e405e6ef35f459 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 18:36:13 +0200 Subject: [PATCH 09/12] doc: Remove section about RPC alias via function pointer This is no longer possible with RPCHelpMan. --- doc/developer-notes.md | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index cebc34d1cc8..6d913467b83 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1429,17 +1429,6 @@ A few guidelines for introducing and reviewing new RPC interfaces: until the wallet is caught up to the chainstate as of the RPC call's entry. This also makes the API much easier for RPC clients to reason about. -- Be aware of RPC method aliases and generally avoid registering the same - callback function pointer for different RPCs. - - - *Rationale*: RPC methods registered with the same function pointer will be - considered aliases and only the first method name will show up in the - `help` RPC command list. - - - *Exception*: Using RPC method aliases may be appropriate in cases where a - new RPC is replacing a deprecated RPC, to avoid both RPCs confusingly - showing up in the command list. - - Use *invalid* bech32 addresses (e.g. in the constant array `EXAMPLE_ADDRESS`) for `RPCExamples` help documentation. From fac8b051979911f036f9156b2d4e7afbe2853336 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 17:42:31 +0200 Subject: [PATCH 10/12] doc: Clarify strprintf size specifier note --- doc/developer-notes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 6d913467b83..59ed36bc3b2 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -972,7 +972,7 @@ Strings and formatting buffer overflows, and surprises with `\0` characters. Also, some C string manipulations tend to act differently depending on platform, or even the user locale. -- For `strprintf`, `LogInfo`, `LogDebug`, etc formatting characters don't need size specifiers. +- For `strprintf`, `LogInfo`, `LogDebug`, etc formatting characters don't need size specifiers (hh, h, l, ll, j, z, t, L) for arithmetic types. - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion. From fab79c1a250db252baa206f59d7e46986e21a57c Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 18:42:29 +0200 Subject: [PATCH 11/12] 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. --- CONTRIBUTING.md | 1 + doc/developer-notes.md | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 25637b40aa9..56843452a1a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -115,6 +115,7 @@ fixes or code moves with actual code changes. Make sure each individual commit is hygienic: that it builds successfully on its own without warnings, errors, regressions, or test failures. +This means tests must be updated in the same commit that changes the behavior. Commit messages should be verbose by default consisting of a short subject line (50 chars max), a blank line and detailed explanatory text as separate diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 59ed36bc3b2..ce6ea7e0596 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -805,9 +805,6 @@ General Bitcoin Core on the master branch. Otherwise, all new pull requests will start failing the tests, resulting in confusion and mayhem. - - *Explanation*: If the test suite is to be updated for a change, this has to - be done first. - Logging ------- From fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 19:02:22 +0200 Subject: [PATCH 12/12] doc: Move CI-must-pass requirement into readme section Seems fine to state it clearly once. --- README.md | 4 ++-- doc/developer-notes.md | 6 ------ 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 6628e0a5c98..7e524461c50 100644 --- a/README.md +++ b/README.md @@ -56,8 +56,8 @@ in Python. These tests can be run (if the [test dependencies](/test) are installed) with: `build/test/functional/test_runner.py` (assuming `build` is your build directory). -The CI (Continuous Integration) systems make sure that every pull request is built for Windows, Linux, and macOS, -and that unit/sanity tests are run automatically. +The CI (Continuous Integration) systems make sure that every pull request is tested on Windows, Linux, and macOS. +The CI must pass on all commits before merge to avoid unrelated CI failures on new pull requests. ### Manual Quality Assurance (QA) Testing diff --git a/doc/developer-notes.md b/doc/developer-notes.md index ce6ea7e0596..27bcd20d003 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -799,12 +799,6 @@ General Bitcoin Core - *Rationale*: RPC allows for better automatic testing. The test suite for the GUI is very limited. -- Make sure pull requests pass CI before merging. - - - *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing - on the master branch. Otherwise, all new pull requests will start failing the tests, resulting in - confusion and mayhem. - Logging -------