4bfcbbfd4a9749f7954e37a7447b5f047465bdf8 doc: remove Security section from build-unix.md (fanquake)
Pull request description:
Our compile documentation isn't the right place for generic binary hardening notes, which are neither particularly Bitcoin-Core specific, or as relevant as they might have once been, i.e non-executable stacks are now just the norm.
Just remove the notes for now, if someone has something more interesting/Bitcoin Core specific, it could be added in separate documentation in the future (maybe into the devwiki or similar).
Split from https://github.com/bitcoin/bitcoin/pull/27685#discussion_r1196517868.
ACKs for top commit:
jarolrod:
ACK 4bfcbbfd4a9749f7954e37a7447b5f047465bdf8
Tree-SHA512: 01b523ba40353d6cafb5dbd6540b514d83a2dc4e462a068fb390ca7de45b78e4a329367f70527f1824006ebe43f8caeea4ad05ec60556d5a5bd0143e27bf6581
6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965 [fuzz] Add MiniMiner target + diff fuzz against BlockAssembler (glozow)
3f3f2d59ea2946a7b7cc8cb0222fb602d62645d0 [unit test] GatherClusters and MiniMiner unit tests (glozow)
59afcc83548ea67a863dac7b75d000bc8f6a7023 Implement Mini version of BlockAssembler to calculate mining scores (glozow)
56484f0fdc44261e723563f59df886d5acdd851f [mempool] find connected mempool entries with GatherClusters(…) (glozow)
Pull request description:
Implement Mini version of BlockAssembler to calculate mining scores
Run the mining algorithm on a subset of the mempool, only disturbing the
mempool to copy out fee information for relevant entries. Intended to be
used by wallet to calculate amounts needed for fee-bumping unconfirmed
transactions.
From comments of sipa and glozow below:
> > In what way does the code added here differ from the real block assembly code?
>
> * Only operates on the relevant transactions rather than full mempool
> * Has the ability to remove transactions that will be replaced so they don't impact their ancestors
> * Does not hold mempool lock outside of the constructor, makes copies of the entries it needs instead (though I'm not sure if this has an effect in practice)
> * Doesn't do the sanity checks like keeping weight within max block weight and `IsFinalTx()`
> * After the block template is built, additionally calculates fees to bump remaining ancestor packages to target feerate
ACKs for top commit:
achow101:
ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
Xekyo:
> ACK [6b605b9](6b605b91c1) modulo `miniminer_overlap` test.
furszy:
ACK 6b605b91 modulo `miniminer_overlap` test.
theStack:
Code-review ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
Tree-SHA512: f86a8b4ae0506858a7b15d90f417ebceea5038b395c05c825e3796123ad3b6cb8a98ebb948521316802a4c6d60ebd7041093356b1e2c2922a06b3b96b3b8acb6
b8ed95127b23873db973b2ef4f68d9f04e9c23ae msvc: Provide `ObjectFileName` explicitly (Hennadii Stepanov)
Pull request description:
This PR is a follow-up to https://github.com/bitcoin/bitcoin/pull/26715.
Fixes intermittent MSVC link [errors](https://cirrus-ci.com/task/6646912535756800).
ACKs for top commit:
sipsorcery:
ACK b8ed95127b23873db973b2ef4f68d9f04e9c23ae.
Tree-SHA512: 4319ecf61b578ce66d240998d089b9bb0a42f89dcfcb1a73f648cd3915f566c773721dcff1feba27d393a743d121334ccb890b1a519173e35a156d6135821ef4
fa4c16b186a34f4172deda617166813c8cb92c59 test: Add test to check tx in the last block can be downloaded (MarcoFalke)
fadc8490ab4fdd91aaf446e08108657b304a3ded test: Split up test_notfound_on_unannounced_tx test case (MarcoFalke)
Pull request description:
If a peer received an `inv` about a transaction, which was included in a block before receiving the corresponding `getdata`, it can be beneficial to send this transaction to the peer to aid compact block relay.
Add a test for this to avoid breaking it in the future.
ACKs for top commit:
sdaftuar:
ACK fa4c16b186a34f4172deda617166813c8cb92c59
instagibbs:
ACK fa4c16b186a34f4172deda617166813c8cb92c59
Tree-SHA512: 1ec16dcc216dd29c849928e753158d45c409612e9ac528db16e5af465bb5b69cd42241ac6f67e5a4583b8e558eeba49fa0a0889a4247b3fb0053b2758eec9490
b53cab0083d99e9610d74517d1d41fc615953770 build: Detect USDT the same way how it is used in the code (Hennadii Stepanov)
Pull request description:
In the code we do not use string literals.
Also a check for `DTRACE_PROBE7` macro has been added as not all systems define`DTRACE_PROBE{6,7,8,9,10,11,12}` macros (e.g., FreeBSD).
ACKs for top commit:
0xB10C:
ACK b53cab0083d99e9610d74517d1d41fc615953770
Tree-SHA512: 74f49424d57bf1929f2b09edba1449cef5a1a2448161952da35302343f3003d5bedeab1417e166b656c5f629303e2de888550b1219e886a1b991b12b9c880794
fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae build: Bump minimum supported GCC to g++-9 (MarcoFalke)
fa69955e741dd60dc6160e81cf223bbecd286806 ci: Bump centos:stream8 to centos:stream9 (MarcoFalke)
fa6a755d9fb22cad3d7063b21a1c8a137ae981b2 ci: Document the false positive error for g++-9 (MarcoFalke)
Pull request description:
It is a bit frustrating to write valid C++ code only to realize that g++-8 fails to parse it later on.
The only non-EOL operating system still shipping with g++-8 is CentOS Stream 8. I think it is reasonable for users of affected Linux distributions to:
* Upgrade their operating system, or compiler to a supported version.
* Alternatively, stay with a previous release of Bitcoin Core as long as it is supported.
Fixes https://github.com/bitcoin/bitcoin/issues/27537
ACKs for top commit:
hebasto:
ACK fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae
fanquake:
ACK fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae
Tree-SHA512: b9cf7e763d3071e1e008c5010de19601d4773afe46d58cf869d3f59285c53240c739a1cd7235a5525ede1bbdf6b6cb6fb091c8fc314864a28d5b27a400bb7632
69d43905b7f1d4849dfaea1b5744ee473ccc8744 test: add coverage for wallet read write db deadlock (furszy)
12daf6fcdcbf5c7f03038338d843df3877714b24 walletdb: scope bdb::EraseRecords under a single db txn (furszy)
043fcb0b053eee6021cc75e3d3f41097f52698c0 wallet: bugfix, GetNewCursor() misses to provide batch ptr to BerkeleyCursor (furszy)
Pull request description:
Decoupled from #26644 so it can closed in favor of #26715.
Basically, with bdb, we can't make a write operation while we are traversing the db with the same db handler. These two operations are performed in different txn contexts and cause a deadlock.
Added coverage by using `EraseRecords()` which is the simplest function that executes this process.
To replicate it, need bdb support and drop the first commit. The test will run forever.
ACKs for top commit:
achow101:
ACK 69d43905b7f1d4849dfaea1b5744ee473ccc8744
hebasto:
re-ACK 69d43905b7f1d4849dfaea1b5744ee473ccc8744
Tree-SHA512: b3773be78925f674e962f4a5c54b398a9d0cfe697148c01c3ec0d68281cc5c1444b38165960d219ef3cf1a57c8ce6427f44a876275958d49bbc0808486e19d7d
faf4315c88d8c81c2ff84870bc81aef3cf719816 test: Return dict in MiniWallet::send_to (MarcoFalke)
Pull request description:
Returning a tuple has many issues:
* If only one value is needed, it can not be indexed by name
* If another value is added to the return value, all call sites need to be updated
Bite the bullet now and update all call sites to fix the above issues.
ACKs for top commit:
brunoerg:
crACK faf4315c88d8c81c2ff84870bc81aef3cf719816
theStack:
Code-review ACK faf4315c88d8c81c2ff84870bc81aef3cf719816
stickies-v:
Code review ACK faf4315c88d8c81c2ff84870bc81aef3cf719816
Tree-SHA512: 8ce1aca237df21f04b3990d0e5fcb49cc408fe6404399d3769a64eae1b5218941157d9785fce1bd9e45140cf70e06c3aa42646ee8f7b57855beb784fc3ef0261
Our compile documentation isn't the right place for genric binary
hardening notes, which are neither particularly Bitcoin-Core specific,
or as relevant as they might have once been, i.e non-executable stacks
are now just the norm.
Just remove the notes for now, if someone has
something more interesting/Bitcoin Core specific, it could be added in
separate documentation in the future (maybe into the devwiki or
similar).
Split from
https://github.com/bitcoin/bitcoin/pull/27685#discussion_r1196517868.
fa29651c3ff3e13a42d6505b14971265562f088a doc: Rework build-unix.md (MarcoFalke)
Pull request description:
The doc has many issues:
* The fist section contains outdated non-existing and confusing configure flags like `--enable-cxx` and `--disable-shared`, as well as edge-case expert options such as `BDB_PREFIX`. Fix that by removing the section and adding notes elsewhere, if applicable.
* There are links to the depends system before instructions on how to simply build from system packages. Fix that by moving that later.
* Also, remove sections that are duplicate with other depends READMEs.
ACKs for top commit:
fanquake:
ACK fa29651c3ff3e13a42d6505b14971265562f088a
TheCharlatan:
ACK fa29651c3ff3e13a42d6505b14971265562f088a
Tree-SHA512: 5348ddf3fa094c630d80b63033ca7b40ec0356427856f9a1075b31244f6bf8ec65cb2a738366e1174ef2fe7e0bf5cc249a62c58f458bbaf50668aceeac954820
a94d75fa81bab8f4695ab1756524e639af0ff69c msvc: Do not define `HAVE_CONSENSUS_LIB` (Hennadii Stepanov)
cf6ff1031bd577c18fe8eb2dd168a6d0d1c16e0d msvc: Clean up `libbitcoin_consensus` source files (Hennadii Stepanov)
30aee016f15b43b90efb023d9bd04aa84cf58ade scripted-diff: Rename `libbitcoinconsensus` to `libbitcoin_consensus` (Hennadii Stepanov)
Pull request description:
The current Autotools-based build system operates with two build artifacts:
- [`LIBBITCOIN_CONSENSUS`](3777c75d14/src/Makefile.am (L31)) which is [defined as](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) "Stable, backwards-compatible consensus functionality used by _libbitcoin_node_ and _libbitcoin_wallet_"
- [`LIBBITCOINCONSENSUS`](3777c75d14/src/Makefile.am (L42)) which is [defined as](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) "Shared library build of static _libbitcoin_consensus_ library"
The way how the `libbitcoinconsensus.vcxproj` project is used in the MSVC build system obviously shows that it is the former use case.
This PR makes the related adjustments to the MSVC build system.
ACKs for top commit:
sipsorcery:
ACK a94d75fa81bab8f4695ab1756524e639af0ff69c.
Tree-SHA512: 1144e13ee2b428ce14be8f76729744830c502a07814eb03e2aa6b8e009d8936fd13743e3f36ef3f31fac0e3979eb9af23e6a1364f151df49b3ae18dbd14cbf99
7014e080154f9c82e4fbe043af292a1e761cdb55 doc: remove mention of glibc 2.10+ (fanquake)
Pull request description:
We already require glibc 2.27+, so mentioning a much older version here is redundant.
ACKs for top commit:
TheCharlatan:
ACK 7014e080154f9c82e4fbe043af292a1e761cdb55
Tree-SHA512: 883a566a80cabe34bfb5d902990f3eca08d0e11438e6c128d311e558f373ec232b0934deb85d12d796baacfeae590af8c73aa1b2faef07f27ffa9011270ffd96
This is required for the next commit. Also, drop CI_RETRY_EXE before
"dnf install", because it requires getopt, which will only be installed
later on via util-linux
fa3761d19d716c2a5b25c76d092b1afbf65ec1d0 ci: Reduce tsan CPU and memory for faster scheduling (MarcoFalke)
aaaa07bc845f0b8741270552e77b9694802fb0e3 ci: Use credits for ARM task (MarcoFalke)
Pull request description:
After https://github.com/bitcoin/bitcoin/pull/27562 the task should finish in less than 10 minutes, so also using credits for it will be cheap and improve dev experience.
ACKs for top commit:
fanquake:
ACK fa3761d19d716c2a5b25c76d092b1afbf65ec1d0
Tree-SHA512: 98ba76eaa63bcbab674076bb9877b3a20d1b90e6d51e2fd7b97ae245414e984d734006b15ea60228a1f4f9bc72fbe2a1a8910fd05e7c52dd2a8b223bfaa25b50
fa199ee614a7ed99c6caf329093a3573ea5a664b ci: Drop NO_WERROR=1 for clang-10 build (MarcoFalke)
fad2c200f46196af69ccd98b6b637d23ceaf8c45 build: Bump minimum Clang to clang-10 (MarcoFalke)
fad7cfee8d6d7bdebd8db950cef09264c91dbb3c doc: Remove outdated CentOS comment (MarcoFalke)
Pull request description:
It doesn't make sense to support a minimum clang version that is difficult to install on all supported operating systems, which generally ship a later version:
* Ubuntu Focal 20.04: https://packages.ubuntu.com/focal/clang-10 and https://packages.ubuntu.com/focal/clang-12
* Debian Bullseye: https://packages.debian.org/bullseye/clang-13
* CentOS 8 Stream: All Clang versions from 11.0 to 15.0
Also, it allows to drop build code, which means it won't waste review when rolling over into cmake (`cmake/module/CheckStdFilesystem.cmake`).
ACKs for top commit:
hebasto:
ACK fa199ee614a7ed99c6caf329093a3573ea5a664b
fanquake:
ACK fa199ee614a7ed99c6caf329093a3573ea5a664b
Tree-SHA512: c1a0e8f191a6db866b8be3c9d254dc3f576fa021e2eaaeb68f3354554a8b38eaa90bbf9871ff92351b715e62a6b7b98cf94eba6dc53d7c951bddb6ad49ba7716
97844d9268b87b5d09b1091bfd0326ed18ce5b91 index: Enable reindex-chainstate with active indexes (Martin Zumsande)
60bec3c82d09ec208bf0028ad32499e251ad205e index: Use first block from locator instead of looking for fork point (Martin Zumsande)
Pull request description:
This makes two improvements to the index init phase:
**1) Prevent index corruption in case a reorg happens when the index was switched off**:
This is done by reading in the top block stored in the locator instead of looking for a fork point already in `BaseIndex::Init()`.
Before, we'd just go back to the fork point by calling `FindForkInGlobalIndex()`, which would have corrupted the coinstatsindex because its saved muhash needs to be reverted step by step by un-applying all blocks in between, which wasn't done before. This is now being done a bit later in `ThreadSync()`, which has existing logic to call the custom `Rewind()` method when going back along the chain to the forking point (thanks ryanofsky for pointing this out to me!).
**2) Allow using the `-reindex-chainstate` option without needing to disabling indexes**:
With `BaseIndex::Init()` not calling `FindForkInGlobalIndex()` anymore, we can allow `reindex-chainstate` with active indexes. `reindex-chainstate` deletes the chain and rebuilds it later in `ThreadImport`, so there is no chain available during `BaseIndex::Init()`, which would lead to problems (see #24789).
But now we'll only need the chain a bit later in `BaseIndex::ThreadSync`, which will wait for the reindex-chainstate in `ThreadImport` to finish and will continue syncing after that.
ACKs for top commit:
ryanofsky:
Code review ACK 97844d9268b87b5d09b1091bfd0326ed18ce5b91. Just simple rebase since last review
Tree-SHA512: e24973fc22e0b87a49026f4820aecb0a4e415f4d381bade9969dd31cf97afecfea0449dce7fcc797343b792199cc8287276d1f5ffa4433dcb54fb24a808db6fb
This is achieved by letting the index sync thread wait until
reindex-chainstate is finished.
This also disables the pruning check when reindexing the chainstate (which is
incompatible with prune mode) because there would be no chain at this point
in init.
The index sync code has logic to go back the chain to the forking point, while
also updating index-specific state, which is necessary to prevent
possible corruption of the coinstatsindex.
Also add a test for this (a reorg happens while the index is deactivated)
that would not pass before this change.
a09269a146b1e32a0e7979692f4455cc2f6faeae guix: document when certain guix patches can be dropped (fanquake)
Pull request description:
Additional notes for when patches can be dropped.
ACKs for top commit:
hebasto:
ACK a09269a146b1e32a0e7979692f4455cc2f6faeae, I have reviewed the changes and they look OK.
jarolrod:
ACK a09269a146b1e32a0e7979692f4455cc2f6faeae
Tree-SHA512: c1876b9a4e3cf73645d25c9077cef19a9b6b7fe2eda5dc9d82fd3ca3f9105453406c1b197e6635035b6ce19c9f255c070bebed5563f68913033d04627202155a
ddddf4957b02c83ed9b6c46b35d8ae1e137889d2 ci: Run iwyu on all src files (MarcoFalke)
Pull request description:
This makes it easier to look at the CI output of a file without having to manually add it first to the list.
ACKs for top commit:
hebasto:
ACK ddddf4957b02c83ed9b6c46b35d8ae1e137889d2
Tree-SHA512: 342b52838ae45ea343731c30058cdd5595d5ea5601a1f396de4466ccdd63f7ab07b3a193df3669e4dca7cb535557dcc98f866b3cf986b98176b20ecead123868
This partially reverts commit 71383f2fad065378393ef55b6d65e14c656b7301.
This should be fine, because if warnings are issues again in the future,
it can be disabled again, along with a list of the false warnings.
fad09b703f5c6d8524a09eef771eb4525f9f3225 ci: Remove unused errtrace trap ERR (MarcoFalke)
Pull request description:
This was added in commit 069752b72613b772a9536a3e7f15fa75097f2946, presumably at a time when the functional tests wouldn't capture stderr.
Now that all tests capture and print stderr on failure, it can be removed. Reference:
* Unit tests capture via `2>&1`:
d7700d3a26/src/Makefile.test.include (L421)
* Functional tests capture as well:
d7700d3a26/test/functional/test_framework/test_node.py (L356)
ACKs for top commit:
fanquake:
ACK fad09b703f5c6d8524a09eef771eb4525f9f3225
hebasto:
ACK fad09b703f5c6d8524a09eef771eb4525f9f3225, tested on Ubuntu 22.04: I can still see warnings from the sanitizers in both unit and functional tests.
Tree-SHA512: 1e786eee432a7a50eb9f78b06b2b157321cc16f91b613e3b476e9e51572592fe4bcf4dc15df176e5f019f24497ac68cf332d2037b55b57498c93f4e19613163c
1b1ffbd014b931afb9435ec10911b9a7c130d3e5 Build: Log when test -f fails in Makefile (TheCharlatan)
541012e621386cd824eed81295206a34ba3ba497 Build: Use AM_V_GEN in Makefiles where appropriate (TheCharlatan)
Pull request description:
This PR triages some behavior around Makefile recipe echoing suppression.
When generating new files as part of the Makefile the recipe is sometimes suppressed with $(AM_V_GEN) and sometimes with `@`. We should prefer $(AM_V_GEN), since this also prints the lines in silent mode. This is arguably more in style with the current recipe echoing.
Before:
`Generated test/data/script_tests.json.h`
Now:
` GEN test/data/script_tests.json.h`
A side effect of this change is that the recipe for generating build.h is now echoed on each make run. Arguably this makes its generation more transparent.
Sometimes the error emitted by `test -f` is currently thrown without any logging. This makes it a bit harder to debug. Instead, print a helpful log message to point the developer in the right direction.
Alternatively this could have been implemented by just removing the recipe echo suppression (@), but the subsequent make output became too noisy.
ACKs for top commit:
fanquake:
ACK 1b1ffbd014b931afb9435ec10911b9a7c130d3e5
Tree-SHA512: e31869fab25e72802b692ce6735f9561912caea903c1577101b64c9cb115c98de01a59300e8ffe7b05b998345c1b64a79226231d7d1453236ac338c62dc9fbb3
e9dcac1ec7b4e00a08a943caa250c4ff00981b8b add `lief` to `spelling.ignore-words` (brunoerg)
258f93000b0522368a315100526ea8e59d0280cd test: fix spelling in `interface_usdt_utxocache` (brunoerg)
Pull request description:
Add `lief` to `spelling.ignore-words` since it's the name of a Python lib and fix spelling error in `interface_usdt_utxocache` (s/eariler/earlier)
ACKs for top commit:
fanquake:
ACK e9dcac1ec7b4e00a08a943caa250c4ff00981b8b
Tree-SHA512: f42cdbb74144c5d289d70bb9bac1179650bb32594678e15f4e18e4b2f68009999d60ff69494d4e68650d82fc1838e67515ed2f922ee84db98735ef906911ec40
0282b2126dcc1216a25417db0716a3a28489b72d walletdb: Remove unused CreateMockWalletDatabase (Andrew Chow)
Pull request description:
This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing.
I thought this was included in #26715, maybe it got lost in a rebase.
ACKs for top commit:
furszy:
utACK 0282b212
brunoerg:
crACK 0282b2126dcc1216a25417db0716a3a28489b72d
Tree-SHA512: 18445c4d8a4e2609ef7471c6d75297f43694b79e768f6c993a5addb1dc0e88bd12bac263c9d8e903d828ddf6bf50434f9e2f72048f4fc528e98fed8ee65123ca
