efb70cd645 doc: correct function name in AssumeUTXO design docs (jrakibi)
Pull request description:
Corrected the function name from `CompleteSnapshotValidation()` to `MaybeCompleteSnapshotValidation()` in the assumeutxo design documentation.
This change ensures that the documentation accurately reflects the actual function name used in the code
ACKs for top commit:
Empact:
ACK efb70cd645
Tree-SHA512: 68b9be3ba710d91a2a955189e227f86b46ccb6a2a13c345d46f276cec6ff12b77ebf9814c4bcb00db7c17e221510e4a2e71175c78a6faf0e0e3159c761bc9b94
fccfdb25b2 doc: Update OpenBSD build docs to 7.4 (Jesse Barton)
Pull request description:
Updated OpenBSD Build doc for 7.4 after testing all build options. No issues on my end.
Also added a note about referring to depends/README.md for detailed instructions on required dependencies.
This was added in reference to a conversation in #29443
ACKs for top commit:
fanquake:
ACK fccfdb25b2
theStack:
lgtm ACK fccfdb25b2
Tree-SHA512: be6d22b605140b37a71e11c5bbed54f60655832d78cd3cb221eddc77c7621a65c0d71baf436f90819be536d9b5dbf1a0b2c82b6b23d62356addc495403f2ba35
Tested and used all build options on OpenBSD 7.4 with no issues.
Added a note about referring to depends/README.md for detailed instructions on required dependencies.
This was added in reference to a conversation in #29443
29029df5c7 [doc] v3 signaling in mempool-replacements.md (glozow)
e643ea795e [fuzz] v3 transactions and sigop-adjusted vsize (glozow)
1fd16b5c62 [functional test] v3 transaction submission (glozow)
27c8786ba9 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke)
9a1fea55b2 [policy/validation] allow v3 transactions with certain restrictions (glozow)
eb8d5a2e7d [policy] add v3 policy rules (glozow)
9a29d470fb [rpc] return full string for package_msg and package-error (glozow)
158623b8e0 [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow)
Pull request description:
See #27463 for overall package relay tracking.
Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340
Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418
Rationale:
- There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2]
- Switching to a cluster-based mempool (see #27677 and #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution.
V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.
Immediate benefits:
- You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later.
- Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction.
This also enables some other cool things (again see #27463 for overall roadmap):
- Ephemeral Anchors
- Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees.
- We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use.
- We can switch to a cluster-based mempool [5] (#27677#28676), which removes CPFP carve out [6].
[1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html
[2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward.
[3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html
[4]: Original PR #25038 also contains a lot of the discussion
[5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7
[6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12
ACKs for top commit:
sdaftuar:
ACK 29029df5c7
achow101:
ACK 29029df5c7
instagibbs:
ACK 29029df5c7 modulo that
Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
25dc87e6f8 libconsensus: deprecate (Cory Fields)
Pull request description:
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib).
Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway.
See for example the discussions:
https://github.com/hebasto/bitcoin/pull/41https://github.com/bitcoin/bitcoin/pull/29123
And here: https://github.com/bitcoin/bitcoin/pull/29180
Where it is pointed out that the libbitcoinconsensus functions are slower than those the internal bitcoind equivalents due to the missing sha2 implementations.
Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.
If there are any users currently using libbitcoinconsensus, please chime in with your use-case!
Edit: Corrected final release to be v27.
ACKs for top commit:
TheCharlatan:
ACK 25dc87e6f8
fanquake:
ACK 25dc87e6f8 - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do.
Tree-SHA512: baff2b3c4f76f520c96021035f751fdcb51bedf00e767660249e92a7bc7c5c176786bcf2c4cfe2d2351c200f932b39eb886bcfb22fbec824a41617590d6a1638
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).
Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.
See for example the discussions:
https://github.com/hebasto/bitcoin/pull/41https://github.com/bitcoin/bitcoin/pull/29123
Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v27. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
0d627c4ca8 doc: refer to "Node relay options" in policy/README (djschnei21)
Pull request description:
Fixed up #29095, to refer to `-help`, rather than listing every option.
ACKs for top commit:
stickies-v:
ACK 0d627c4ca8
glozow:
lgtm ACK 0d627c4ca8
Tree-SHA512: 37d36ffa48297371eb0032ed48dce28802f862f6c18bdb50207555a228ce252e51a93a6fdef86b3e596d486c5107594d64db89f077b77fc885fe84cecb1dadc3
aaaace2fd1 fuzz: Assume presence of __builtin_*_overflow, without checks (MarcoFalke)
fa223ba5eb Revert "build: Fix undefined reference to __mulodi4" (MarcoFalke)
fa7c751bd9 build: Bump clang minimum supported version to 14 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-14 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
For reference:
* https://packages.debian.org/bookworm/clang (`clang-14`)
* https://packages.ubuntu.com/jammy/clang (`clang-14`)
* CentOS-like 8/9 Stream: All Clang versions from 15 to 17
* FreeBSD 12/13: All Clang versions from 15 to 16
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang17`); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bullseye/g++ (g++-10)
* https://packages.ubuntu.com/focal/g++-10
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
ACKs for top commit:
fanquake:
ACK aaaace2fd1
Tree-SHA512: 81d066b14cc568d27312f1cc814b09540b038a10a0a8e9d71fc9745b024fb6c32a959af673e6819b817ea7cef98da4abfa63dff16cffb7821b40083016b0291f
e60fc7d5d3 logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a056 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e329 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012 logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874 logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615 logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d3🌚
achow101:
ACK e60fc7d5d3
stickies-v:
ACK e60fc7d5d3
jamesob:
ACK e60fc7d5d3 ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
These provide simple and clear ways to write the most common logging
operations:
LogInfo("msg");
LogDebug(BCLog::LogFlags::NET, "msg");
LogError("msg");
LogWarning("msg");
LogTrace(BCLog::LogFlags::NET, "msg");
For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.
6666713041 refactor: Rename fs::path::u8string() to fs::path::utf8string() (MarcoFalke)
856c88776f ArgsManager: return path by value from GetBlocksDirPath() (Vasil Dimov)
fa3d9304e8 refactor: Remove pre-C++20 fs code (MarcoFalke)
fa00098e1a Add tests for C++20 std::u8string (MarcoFalke)
fa2bac08c2 refactor: Avoid copy/move in fs.h (MarcoFalke)
faea30227b refactor: Use C++20 std::chrono::days (MarcoFalke)
Pull request description:
This:
* Removes dead code.
* Avoids unused copies in some places.
* Adds copies in other places for safety.
ACKs for top commit:
achow101:
ACK 6666713041
ryanofsky:
Code review ACK 6666713041. Just documentation change since last review.
stickies-v:
re-ACK 6666713041
Tree-SHA512: 6176e44f30b310d51632ec2d3827c3819905d0ddc6a4b57acfcb6cfa1f9735176da75ee8ed4a4abd1296cb0b83bee9374cc6f91ffac87c19b63c435eeadf3f46
fa88953d6f doc: Add link to needs-release-notes label (MarcoFalke)
Pull request description:
This makes it easier to spot and not forget. C.f. https://github.com/bitcoin/bitcoin/pull/28597#issuecomment-1845299642
ACKs for top commit:
kristapsk:
ACK fa88953d6f
TheCharlatan:
ACK fa88953d6f
Tree-SHA512: 28336cde36d62622d1b6627497291cbd4644bf1e4e6f17dc9cde39f254e7094dd02484da754e45558e59facb20941dd0c049ce7b33dcc62bfec6c26c16516cdf
fa6b053b5c mempool: persist with XOR (MarcoFalke)
Pull request description:
Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.
While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or may cause block relay to be slower.
Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a random XOR pattern over the dat file when writing or reading it.
Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat file. Any program that intentionally wants to mess with the dat file can still trivially do so.
ACKs for top commit:
achow101:
re-ACK fa6b053b5c
glozow:
reACK fa6b053b5c
ismaelsadeeq:
ACK fa6b053b5c
Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e
3b70f7b615 doc: fix broken doc/design/multiprocess.md links after #24352 (Ryan Ofsky)
6d43aad742 span: Make Span template deduction guides work in SFINAE context (Ryan Ofsky)
8062c3bdb9 util: Add ArgsManager SetConfigFilePath method (Ryan Ofsky)
441d00c60f interfaces: Rename CalculateBumpFees methods to be compatible with capn'proto (Ryan Ofsky)
156f49d682 interfaces: Change getUnspentOutput return type to avoid multiprocess segfault (Ryan Ofsky)
4978754c00 interfaces: Add schedulerMockForward method so mockscheduler RPC can work across processes (Ryan Ofsky)
924327eaf3 interfaces: Fix const virtual method that breaks multiprocess support (Ryan Ofsky)
82a379eca8 streams: Add SpanReader ignore method (Russell Yanofsky)
Pull request description:
This is a collection of small changes to interfaces and code which were needed as part of multiprocess PR #10102, but have been moved here to make that PR smaller.
All of these changes are refactoring changes which do not affect behavior of current code
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 3b70f7b615
naumenkogs:
ACK 3b70f7b615
maflcko:
re-ACK 3b70f7b615🎆
Tree-SHA512: 2368772b887056ad8a9f84c299cfde76ba45943770e3b5353130580900afa9611302195b899ced7b6e303b11f053ff204cae7c28ff4e12c55562fcc81119ba4c
3c208cc05e Add offline signing tutorial (Brandon Odiwuor)
Pull request description:
This PR adds offline signing tutorial. Fixes https://github.com/bitcoin/bitcoin/issues/9492
Although there currently exists tutorials on external-signer and on multisig implemented on #24519 . The external-signer tutorial assumes a connected device and the multisig tutorial is only for multisig transactions and does not include using an offline wallet
- The tutorial uses signet(instead of regtest) to be as close as possible to mainnet
ACKs for top commit:
achow101:
ACK 3c208cc05e
willcl-ark:
ACK 3c208cc05e
pinheadmz:
ACK 3c208cc05e
Zero-1729:
ACK 3c208cc05e
Tree-SHA512: c1686043d9e9ed440e78d219a6b18d58d62efd05bdd535e74194d8cc2db0a91e94c6c619106453120a137e47220cf3ab27af3214e861f4e5cc419a73a8704dd6
91d0888921 sync: unpublish LocksHeld() which is used only in sync.cpp (Vasil Dimov)
3df37e0c78 doc: clarify that LOCK() does AssertLockNotHeld() internally (Vasil Dimov)
Pull request description:
Constructs like
```cpp
AssertLockNotHeld(m);
LOCK(m);
```
are equivalent to (almost, modulo some logging differences, see below)
```cpp
LOCK(m);
```
for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in such cases. Requests to do the former keep coming during review process. `developer-notes.md` explicitly states "Combine annotations in function declarations with run-time asserts in function definitions", but that seems to be too strong or unclear. `LOCK()` is also a run-time assert in this case.
Also remove `LocksHeld()` from the public interface in `sync.h` since it is only used in `sync.cpp`.
ACKs for top commit:
achow101:
ACK 91d0888921
hebasto:
ACK 91d0888921, I have reviewed the code and it looks OK.
Tree-SHA512: c4b7ef2c0bfeb28d1c4f55f497810f629873137e02f5a92137c02cb1ff603ac76473dcd2171e594491494a5cb87b8c0c803e06b86f190d4acb231791e28e802d
fa25e8b0a1 doc: Recommend lint image build on every call (MarcoFalke)
faf70c1f33 Bump python minimum version to 3.9 (MarcoFalke)
fa8996b930 ci: Bump i686_multiprocess.sh to latest Ubuntu LTS (MarcoFalke)
Pull request description:
All supported operating systems ship with python 3.9 (or later), so bumping the minimum should not cause any issues. A bump will allow new code to use new python 3.9 features.
For reference:
* https://packages.debian.org/bullseye/python3
* https://packages.ubuntu.com/focal/python3.9
* FreeBSD 12/13 also ships with 3.9
* CentOS-like 8/9 also ships with 3.9 (and 3.11)
* OpenSuse Leap also ships with 3.9 (and 3.11) https://software.opensuse.org/package/python311-base
This is for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month.
ACKs for top commit:
Sjors:
ACK fa25e8b0a1
jamesob:
ACK fa25e8b0a1 ([`jamesob/ackr/28211.1.MarcoFalke.bump_python_minimum_supp`](https://github.com/jamesob/bitcoin/tree/ackr/28211.1.MarcoFalke.bump_python_minimum_supp))
Tree-SHA512: 86c9f6ac4b5ba94a62ee6a6062dd48a8295d8611a39cdb5829f4f0dbc77aaa1a51edccc7a99275bf699143ad3a6fe826de426d413e5a465e3b0e82b86d10c32e
4bfaad4eca chainparams, assumeutxo: Fix signet txoutset hash (Fabian Jahr)
a503cd0f0b chainparams, assumeutxo: Fix testnet txoutset hash (Fabian Jahr)
f6213929c5 assumeutxo: Check deserialized coins for out of range values (Fabian Jahr)
66865446a7 docs: Add release notes for #28685 (Fabian Jahr)
cb0336817e scripted-diff: Rename hash_serialized_2 to hash_serialized_3 (Fabian Jahr)
351370a1d2 coinstats: Fix hash_serialized2 calculation (Fabian Jahr)
Pull request description:
Closes#28675
The last commit demonstrates that theStack's analysis [here](https://github.com/bitcoin/bitcoin/issues/28675#issuecomment-1770389468) seems to be correct. There will be more changes needed for the rest of the test suite but the `feature_assumeutxo.py` with my additional tests pass.
ACKs for top commit:
achow101:
ACK 4bfaad4eca
theStack:
Code-review ACK 4bfaad4eca
ryanofsky:
Code review ACK 4bfaad4eca
Tree-SHA512: 2f6abc92b282f7c5da46391803cf0804d13978d191d541f2509b532c538abccd0a081e46cda23d80d47206a05fa2b5d41b7ab246e6a263db7a7461d6292116ef
03f82087f6 doc: assumeutxo prune and index notes (Sjors Provoost)
Pull request description:
Based on recent comments on #27596.
ACKs for top commit:
pablomartin4btc:
re ACK 03f82087f6
ryanofsky:
ACK 03f82087f6. Nice changes, these seem like very helpful notes
Tree-SHA512: fe651b49f4d667400a3655899f27a96dd1eaf67cf9215fb35db5f44fb8c0313e7d541518be6791fec93392df24b909793f3886adb808e53228ed2a291165639d
