Inside Bitcoin

1001
1002  - Prefer locks that are in a class rather than global, and that are
1003    internal to a class (private or protected) rather than public.
1004
1005  - Combine annotations in function declarations with run-time asserts in
1006    function definitions (`AssertLockNotHeld()` can be omitted if `LOCK()` is
1007    called unconditionally after it because `LOCK()` does the same check as
1008    `AssertLockNotHeld()` internally, for non-recursive mutexes):
1009
1010```C++
1011// txmempool.h
1012class CTxMemPool
1013{
1014public:
1015    ...
1016    mutable RecursiveMutex cs;
1017    ...
1018    void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs);
1019    ...
1020}
1021
1022// txmempool.cpp
1023void CTxMemPool::UpdateTransactionsFromBlock(...)
1024{
1025    AssertLockHeld(::cs_main);
1026    AssertLockHeld(cs);
1027    ...
1028}
1029```
1030
1031```C++
1032// validation.h
1033class Chainstate
1034{
1035protected:
1036    ...
1037    Mutex m_chainstate_mutex;
1038    ...
1039public:
1040    ...
1041    bool ActivateBestChain(
1042        BlockValidationState& state,
1043        std::shared_ptr<const CBlock> pblock = nullptr)
1044        EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
1045        LOCKS_EXCLUDED(::cs_main);
1046    ...
1047    bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex)
1048        EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
1049        LOCKS_EXCLUDED(::cs_main);
1050    ...
1051}
1052
1053// validation.cpp
1054bool Chainstate::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex)
1055{
1056    AssertLockNotHeld(m_chainstate_mutex);
1057    AssertLockNotHeld(::cs_main);
1058    {
1059        LOCK(cs_main);
1060        ...
1061    }
1062
1063    return ActivateBestChain(state, std::shared_ptr<const CBlock>());
1064}
1065```
1066
1067- Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
1068  deadlocks are introduced. This is defined by default when
1069  building with `-DCMAKE_BUILD_TYPE=Debug`.
1070
1071- When using `LOCK`/`TRY_LOCK` be aware that the lock exists in the context of
1072  the current scope, so surround the statement and the code that needs the lock
1073  with braces.
1074
1075  OK:
1076```c++
1077{
1078    TRY_LOCK(cs_vNodes, lockNodes);
1079    ...
1080}
1081```
1082
1083  Wrong:
1084```c++
1085TRY_LOCK(cs_vNodes, lockNodes);
1086{
1087    ...
1088}
1089```
1090
1091## Scripts
1092
1093Write scripts in Python or Rust rather than bash, when possible.
1094
1095## Source code organization
1096
1097- Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or
1098  when performance due to inlining is critical.
1099
1100  - *Rationale*: Shorter and simpler header files are easier to read and reduce compile time.
1101
1102- Every `.cpp` and `.h` file should `#include` every header file it directly uses classes, functions or other
1103  definitions from, even if those headers are already included indirectly through other headers.
1104
1105  - *Rationale*: Excluding headers because they are already indirectly included results in compilation
1106    failures when those indirect dependencies change. Furthermore, it obscures what the real code
1107    dependencies are. The [Using IWYU](#using-iwyu) section describes a tool to help enforce this.
1108
1109- Don't import anything into the global namespace (`using namespace ...`). Use
1110  fully specified types such as `std::string`.
1111
1112  - *Rationale*: Avoids symbol conflicts.
1113
1114- Terminate namespaces with a comment (`// namespace mynamespace`). The comment
1115  should be placed on the same line as the brace closing the namespace, e.g.
1116
1117```c++
1118namespace mynamespace {
1119...
1120} // namespace mynamespace
1121
1122namespace {
1123...
1124} // namespace
1125```
1126
1127  - *Rationale*: Avoids confusion about the namespace context.
1128
1129## GUI
1130
1131- Do not display or manipulate dialogs in model code (classes `*Model`).
1132
1133  - *Rationale*: Model classes pass through events and data from the core, they
1134    should not interact with the user. That's where View classes come in. The converse also
1135    holds: try to not directly access core data structures from Views.
1136
1137- Avoid adding slow or blocking code in the GUI thread. In particular, do not
1138  add new `interfaces::Node` and `interfaces::Wallet` method calls, even if they
1139  may be fast now, in case they are changed to lock or communicate across
1140  processes in the future.
1141
1142  Prefer to offload work from the GUI thread to worker threads (see
1143  `RPCExecutor` in console code as an example) or take other steps (see
1144  https://doc.qt.io/archives/qq/qq27-responsive-guis.html) to keep the GUI
1145  responsive.
1146
1147  - *Rationale*: Blocking the GUI thread can increase latency, and lead to
1148    hangs and deadlocks.
1149
1150## Subtrees
1151
1152Several parts of the repository are subtrees of software maintained elsewhere.
1153
1154Normally, these are maintained by active developers of Bitcoin Core, in which case
1155changes should go directly upstream without being PRed directly against the project.
1156They will be merged back in the next subtree merge.
1157
1158Others are external projects without a tight relationship with our project. Changes
1159to these should also be sent upstream, but bugfixes may also be prudent to PR against
1160a Bitcoin Core subtree, so that they can be integrated quickly. Cosmetic changes
1161should be taken upstream.
1162
1163There is a tool in `test/lint/git-subtree-check.sh` ([instructions](../test/lint#git-subtree-checksh))
1164to check a subtree directory for consistency with its upstream repository.
1165
1166The tool instructions also include a list of the subtrees managed by Bitcoin Core.
1167
1168To fully verify or update a subtree, add it as a remote:
1169
1170```sh
1171git remote add libmultiprocess https://github.com/bitcoin-core/libmultiprocess.git
1172```
1173
1174To update the subtree:
1175
1176```sh
1177git fetch libmultiprocess
1178git subtree pull --prefix=src/ipc/libmultiprocess libmultiprocess master --squash
1179```
1180
1181The ultimate upstream of the few externally managed subtrees are:
1182
1183- src/leveldb
1184  - Upstream at https://github.com/google/leveldb ; maintained by Google. Open
1185    important PRs to the Bitcoin Core subtree to avoid delay.
1186  - **Note**: Follow the instructions in [Upgrading LevelDB](#upgrading-leveldb) when
1187    merging upstream changes to the LevelDB subtree.
1188
1189- src/crc32c
1190  - Used by leveldb for hardware acceleration of CRC32C checksums for data integrity.
1191  - Upstream at https://github.com/google/crc32c ; maintained by Google.
1192
1193## Upgrading LevelDB
1194
1195Extra care must be taken when upgrading LevelDB. This section explains issues
1196you must be aware of.
1197
1198### File Descriptor Counts
1199
1200In most configurations, we use the default LevelDB value for `max_open_files`,
1201which is 1000 at the time of this writing. If LevelDB actually uses this many
1202file descriptors, it will cause problems with Bitcoin's `select()` loop, because
1203it may cause new sockets to be created where the fd value is >= 1024. For this
1204reason, on 64-bit Unix systems, we rely on an internal LevelDB optimization that
1205uses `mmap()` + `close()` to open table files without actually retaining
1206references to the table file descriptors. If you are upgrading LevelDB, you must
1207sanity check the changes to make sure that this assumption remains valid.
1208
1209In addition to reviewing the upstream changes in `env_posix.cc`, you can use `lsof` to
1210check this. For example, on Linux this command will show open `.ldb` file counts:
1211
1212```bash
1213$ lsof -p $(pidof bitcoind) |\
1214    awk 'BEGIN { fd=0; mem=0; } /ldb$/ { if ($4 == "mem") mem++; else fd++ } END { printf "mem = %s, fd = %s\n", mem, fd}'
1215mem = 119, fd = 0
1216```
1217
1218The `mem` value shows how many files are mmap'ed, and the `fd` value shows how
1219many file descriptors these files are using. You should check that `fd` is a
1220small number (usually 0 on 64-bit hosts).
1221
1222See the notes in the `SetMaxOpenFiles()` function in `dbwrapper.cc` for more
1223details.
1224
1225### Consensus Compatibility
1226
1227It is possible for LevelDB changes to inadvertently change consensus
1228compatibility between nodes. This happened in Bitcoin 0.8 (when LevelDB was
1229first introduced). When upgrading LevelDB, you should review the upstream changes
1230to check for issues affecting consensus compatibility.
1231
1232For example, if LevelDB had a bug that accidentally prevented a key from being
1233returned in an edge case, and that bug was fixed upstream, the bug "fix" would
1234be an incompatible consensus change. In this situation, the correct behavior
1235would be to revert the upstream fix before applying the updates to Bitcoin's
1236copy of LevelDB. In general, you should be wary of any upstream changes affecting
1237what data is returned from LevelDB queries.
1238
1239## Scripted diffs
1240
1241For reformatting and refactoring commits where the changes can be easily automated using a bash script, we use
1242scripted-diff commits. The bash script is included in the commit message and our CI job checks that
1243the result of the script is identical to the commit. This aids reviewers since they can verify that the script
1244does exactly what it is supposed to do. It is also helpful for rebasing (since the same script can just be re-run
1245on the new master commit).
1246
1247To create a scripted-diff:
1248
1249- start the commit message with `scripted-diff:` (and then a description of the diff on the same line)
1250- in the commit message include the bash script between lines containing just the following text: