7378f27b4f test: run utxo-to-sqlite script test with spk/txid format option combinations (Sebastian Falbesoner)
b30fca7498 contrib: utxo_to_sqlite.py: add options to store txid/spk as BLOBs (Sebastian Falbesoner)
Pull request description:
This PR is a late follow-up to https://github.com/bitcoin/bitcoin/pull/27432, introducing an option for the utxo-to-sqlite script to store the txid/scriptPubKey columns as bytes (= `BLOB` storage class in sqlite, see e.g. https://www.sqlite.org/datatype3.html in sqlite) rather than hex strings. This was proposed in earlier reviews (https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516857024, https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653739351) and has the obvious advantage of a significantly smaller size of the resulting database (and with that, faster conversion) and the avoidance of hex-to-bytes conversion for further processing of the data [1]. The rationale on why hex strings were chosen back then (and still stays the default, if only for compatibility reasons) is laid out in https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516922824 [2].
The approach taken is introducing new parameters `--spk` and `--txid` which can either have the values "hex", "raw" (for scriptpubkey) and "hex", "raw", "rawle" (for txid). Thanks to ajtowns for providing this suggestion. Happy to take further inputs on naming and thoughts on future extensibility etc.
[1] For a concrete example, I found that having these columns as bytes would be nice while working on a SwiftSync hints generator tool (https://github.com/theStack/swiftsync-hints-gen), which takes the result of the utxo-to-sqlite tool as input.
[2] note that in contrast what I wrote back then, I think there is no ambiguity on byte-string-serialization of txids; they are ultimately just hash results and hence, they should be stored as such, and adding a big/little endian knob wouldn't make much sense. The drawback of not being able to immediately show txid-strings (as one would need to do the bytes-reversal step first, which is not possible in sqlite, see e.g. https://github.com/bitcoin/bitcoin/pull/24952#issuecomment-1165499803) still remains though.
ACKs for top commit:
ajtowns:
ACK 7378f27b4f
w0xlt:
reACK 7378f27b4f
sedited:
ACK 7378f27b4f
Tree-SHA512: 265991a1f00e3d69e06dd9adc34684720affd416042789db2d76226e4b31cf20adc433a74d14140f17739707dee57e6703f72c20bd0f8dd08b6d383d3f28b450
37cc2a2d95 logging: use util/log.h where possible (stickies-v)
bb8e9e7c4c logging: Move message formatting to util/log.h (stickies-v)
001f0a428e move-only: Move logging macros to util/log.h (stickies-v)
94c0adf4e8 move-onlyish: Move logging levels to util/log.h (stickies-v)
56d113cab0 move-only: move logging categories to logging/categories.h (stickies-v)
f5233f7e98 move-only: Move SourceLocation to util/log.h (stickies-v)
Pull request description:
This is a mostly move-only change. It's a small refactoring that allows logging macros to be used by including a simple `util/log.h` header instead of the full `logging.h` logging implementation. Most of the changes here were cherry-picked from #34374.
Original motivation for this change was to reduce the size and complexity of #34374 (kernel structured logging PR) and reduce the number of conflicts it causes with other PRs. But this should also make sense as a standalone change to have a clearer separation of concerns between log generation and log handling, and avoid needing to depend on the whole logging framework in call sites that only emit log messages.
Recommended to review with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
l0rinc:
diff ACK 37cc2a2d95
stickies-v:
re-ACK 37cc2a2d95
optout21:
crACK 37cc2a2d95
sedited:
ACK 37cc2a2d95
Tree-SHA512: c7a761323ae63f07ad290d4e3766ba1348a132c8cc68a9895fa9ae5c89206599c00646c42ef77223ac757b9d8bfe6c181bead15e7058e4d8966b3bac88a8f950
b73a62f667 test: Ensure invalid block was processed before checking debug.log (Ava Chow)
Pull request description:
Prior to merging a PR, I run 4 different build configurations and their tests in parallel, and consistently one of those will have `feature_assumevalid.py` fail. The failure is because the `assert_debug_log` context exits before the invalid block is processed, so the lines it is looking for don't appear in the part of the log that it is examining.
This PR should resolve that issue by waiting for `getchaintips` to report that the invalid chain is invalid before exiting the `assert_debug_log` context.
ACKs for top commit:
l0rinc:
tested ACK b73a62f667
sedited:
ACK b73a62f667
Tree-SHA512: 96409ed55f686961c47949d31569d6be8e424d952883c92a9135a4e8a795047d4e2a86c9d27ef5653d21780717275434b0bca1586059cfd5088cd2c867c7baea
576f892027 qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file (Hennadii Stepanov)
4b9f5beafe Update Transifex slug for 31.x (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](46e1288df2/doc/release-process.md).
It is required to open Transifex translations for v31.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/33607.
For reference, see the previous similar PR: https://github.com/bitcoin/bitcoin/pull/33152.
**Note for reviewers:**
To reproduce the diff in the last commit, run:
```
cmake --preset dev-mode
cmake --build build_dev_mode --target translate
```
ACKs for top commit:
stickies-v:
ACK 576f892027
Tree-SHA512: 9875831b8ea6ace5b6e47fe10351bc7cdd26b9c27fe275f678c7f33f6df115ce3e956f9d522905b2fda76d82fc0a4135482cfb665c4428ab2bc1164c49cf82f4
633d183119 test: misc interface_ipc_mining.py improvements (Sjors Provoost)
52ccd9215e test: split interface_ipc_mining.py into subtests (Sjors Provoost)
4e49fa2a68 test: add interface_ipc_mining.py (Sjors Provoost)
01a1ae889e test: move IPC helpers to ipc_util.py (Sjors Provoost)
Pull request description:
This test has been growing too large, making it difficult to maintain. Especially when multiple pull requests change it.
- move helper functions to `ipc_util.py`
- move mining test to `interface_ipc_mining.py`, keeping only an interface sanity check in `interface_ipc.py`
- split the tests in `interface_ipc_mining.py`
- misc tweaks (to reduce churn in the above commits)
Review hint:
```sh
git show --color-moved=dimmed-zebra --color-moved-ws=ignore-space-change
```
ACKs for top commit:
janb84:
re ACK 633d183119
fjahr:
reACK 633d183119
l0rinc:
code review ACK 633d183119
sedited:
Re-ACK 633d183119
Tree-SHA512: 81bbbec1f03a6ff63068dbefc1bc4768fcd24313d84ba454bb2494fe4a65838dbaeb93ad7f02ed4c9932394f3d24638453bb51ce0e05f561dc613414beda37e4
fa0677d131 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb3956 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec2 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735 test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131
achow101:
ACK fa0677d131
janb84:
re ACK fa0677d131
sipa:
crACK fa0677d131
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
fa43897c1d doc: Fix LLM nits in net_processing.cpp (MarcoFalke)
bbbba0fd4b scripted-diff: Use references when nullptr is not possible (MarcoFalke)
fac5415466 refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages (MarcoFalke)
fac529188e refactor: Pass Peer& to ProcessMessage (MarcoFalke)
fa376095a0 refactor: Pass CNode& to ProcessMessages and SendMessages (MarcoFalke)
fada838014 refactor: Make ProcessMessage private again (MarcoFalke)
fa80cd3cee test: [refactor] Avoid calling private ProcessMessage() function (MarcoFalke)
Pull request description:
There is a single unit test, which calls the internal `ProcessMessage` function. This is problematic, because it makes future changes harder, since they will need to carry over this public internal interface each time.
Also, there is a mixed use of pointers and references in p2p code, where just based on context, a pointer may sometimes assumed to be null, or non-null. This is confusing when reading the code, or making or reading future changes.
Fix both issues in a series of commits, to:
* refactor the single unit test to call higher-level functions
* Make `ProcessMessage` private again
* Use references instead of implicit non-null pointers, mostly in a scripted-diff
ACKs for top commit:
optout21:
reACK fa43897c1d
ajtowns:
ACK fa43897c1d
Crypt-iQ:
crACK fa43897c1d
achow101:
ACK fa43897c1d
Tree-SHA512: d03d8ea35490a995f121be3d2f3e4a22d1aadfeab30bc42c4f8383dab0e6e27046260e792d9e5a94faa6777490ba036e39c71c50611a38f70b90e3a01f002c9e
48161f6a05 wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1 wallet: remove PreSelectedInputs (stratospher)
7819da2c16 walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01 wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e3 wallet: ensure COutput added in set are unique (stratospher)
fefa3be782 wallet: fix, make 'total_effective_amount' optional actually optional (stratospher)
Pull request description:
picks up https://github.com/bitcoin/bitcoin/pull/25269.
This PR re-implements the code path so that an error message is thrown when a transaction's total amount (including fees) exceeds the available balance. It also refactors the wallet's coin selection code.
1. the first 3 commits are unrelated to the code but few small bug fixes which are nice to fix. but also kind of impacts the remaining logic. (could PR separately if reviewers wish)
1. c467325aaf: make `total_effective_amount` optional actually optional
2. 2202ab597596c84fc49f8784e823372b7a9efcbe: ensure `set<shared_ptr<COutput>>` has unique COutput
3. a5ffbbf122d66fc4ad9b2e7c6d7d1dfa1816388e: Correctly reserve size when flattening `CoinsResult.coins` map to vector
3. the next 3 commits from 4745d5480ca5c3809edd51140e4d2c0433582844 replace the `PreSelectedInputs` struct with `CoinsResult` and removes `PreSelectedInputs`.
4. the last commit (e664484a6d34c1795ebb0925ab31faea5d64ab00) deals with the error message - `AmountWithFeeExceedsBalance` error inside `WalletModel::prepareTransaction` is never thrown and remains an unused code path. This is because `createTransaction` does not retrieve the fee when the process fails. The fee return arg is set only at the end of the process, when the transaction is successfully created. Therefore, if the transaction creation fails, the fee is not available inside `WalletModel::prepareTransaction` to trigger the `AmountWithFeeExceedsBalance` error.
This PR re-implements the feature inside `CreateTransactionInternal` and adds test coverage for it.
| on master | on PR |
|-----------|-------|
| <img src="https://github.com/user-attachments/assets/a903e687-2466-42c7-b898-5dec24bfe515" width="750" alt="Insufficient funds" /> | <img src="https://github.com/user-attachments/assets/74bb3c83-6132-4c09-91f0-0a446618b3c8" width="750" alt="AmountWithFeeExceedsBalance" /> |
the unreachable code path is removed in https://github.com/bitcoin-core/gui/pull/807 which requires this PR.
ACKs for top commit:
achow101:
ACK 48161f6a05
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
In feature_assumevalid.py, we check that a modified block 102 is invalid
by asserting a message in the debug.log. However, this can
intermittently fail as exiting the assert_debug_log can occur before the
block has actually been validated, thus causing the test to fail as the
validation error message is not present in the chunk of the debug.log
being examined.
We can wait for the block to make an invalid chain tip to ensure that the log
line will be present.
Split the Mining interface test into focused subtests.
Keep the initial tip-change pre-mine check in run_mining_interface_test.
As a result run_block_template_test no longer has newblockref.
Split Mining interface tests into interface_ipc_mining.py and keep
interface_ipc.py for echo + simple inspectors.
Register the new test in test_runner.py.
The setup code around "Create Mining proxy object" is duplicated
in the new test file, but the simple insector checks below it
are not moved.
d79249d279 ci: add chimera Linux LTO CI job (fanquake)
Pull request description:
Adds a CI config based on using [Chimera Linux](https://chimera-linux.org/). This might be interesting for any of the following:
* Chimera is based on LLVM & musl libc - we test both of these in isolation, but not together.
* No GNU components. I don't think we have an existing Linux CI job that doesn't have a gcc/stdlibc++ install. This exercises the depends logic for a fully LLVM/Clang/lld only build, including building the native tools (related to #33902).
* We don't currently have a job with LTO enabled (here using CMakes `CMAKE_INTERPROCEDURAL_OPTIMIZATION`, which is `-flto=thin` for LLVM/Clang). I think this is worth having generally (we do use LTO in some other places, like oss-fuzz). If runtime is too much of an issue, then it could also be dropped. (Chimera itself is also compiled with LTO).
QT in depends doesn't build (#32744), so is excluded for now.
Chimera has pointed out at least a few quirks, i.e #34390, #34408 and https://github.com/bitcoin/bitcoin/pull/29963#discussion_r2707922298.
ACKs for top commit:
maflcko:
lgtm ACK d79249d279
hebasto:
ACK d79249d279.
Tree-SHA512: 1174a7462bf2e7433a2c27a6cf398e94b05db42bb414629c71cf9f9a297ca269e173ae1b7517b30510b494b4397f918eef706d3c75c4286767c5557aeb6db4c7
b65a3d8009 iwyu: Fix patch to prefer `<cstdint>` (Hennadii Stepanov)
Pull request description:
The goal of the [patch](https://github.com/bitcoin/bitcoin/blob/master/ci/test/01_iwyu.patch) is to suggest C++ headers rather than their C counterparts. However, for fixed width integer types, the patched IWYU currently suggests `<cinttypes>` where `<cstdint>` is sufficient.
This PR fixes this behavior.
ACKs for top commit:
maflcko:
lgtm ACK b65a3d8009
furszy:
utACK b65a3d8009
willcl-ark:
utACK b65a3d8009
Tree-SHA512: 695efdd44b92a642401738572e49c8b6591aa4463d387107fdf3d2f7c9c4b39f4097cb82413752caf9e8890dcca7246a894e562a1dd17023b05a7e455705beac
PreSelectedInputs is confusing to use. it's `total_amount`
might store total amount or effective amount based on SFFO.
ex: we might accidentally sum preselected inputs effective
amount (named `total_amount`) with automatically selected
inputs actual total amount.
CoinsResult has a cleaner interface with separate fields
for both these amounts.
2 behavioural changes:
1. no more default assert error if effective value is unset
- previously PreSelectedInputs::Insert() called
COutput::GetEffectiveValue() which assert failed
if the optional was unset.
- now we don't default assert anymore.
* in GUI/getAvailableBalance better not to assert.
* SelectCoins's preselected inputs always contain a
feerate, so effective amount should be set.
explicitly added an assertion to ensure this.
2. FetchSelectedInputs uses OutputType::UNKNOWN as key to
populate CoinsResult's coins map. it's discarded later.
faa4ab113c ci: Drop valgrind fuzz from GHA matrix (MarcoFalke)
Pull request description:
The valgrind fuzz task is problematic, because:
* It is redundant with the msan fuzz task, which has std lib hardening enabled, so often UB is diagnosed before it even happens in the valgrind task.
* All issues so far found by the valgrind fuzz task were also found by the hardened msan fuzz task.
* All other issues were false-positives, which are hard to debug, and confusing and tedious to work around.
I don't think there is any value in asking pull request authors to debug valgrind false-positives that they triggered by accident. So remove the task for now.
I know that there are some devs, who like to keep the task, but if the task is kept, it should come with clear instructions on how to deal with false-postives in pull requests.
I am not proposing to remove the config itself, and I am happy to continue maintaining it, like it was done before. However, as of now, running it in the GHA matrix is of negative or questionable benefit.
ACKs for top commit:
l0rinc:
ACK faa4ab113c
fanquake:
ACK faa4ab113c - hopefully we can revisit re-adding soon. To be clear, I don't agree with the rationale from #34304, or the initial changes there. The case here, and the fact that it is causing disruption in this repo, is more pressing.
Tree-SHA512: 59272f4b5b01c3b8ee6078ea635441f17776d4d8923f1adacdabdbb00bd2eb0234b30dc5b27938e29f8e79b3c3bebed5f339ae36c2c8fb17ea9d3a2884bee986
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
returns the total amount (if SFFO), otherwise the effective amount.
previously, this was the logic in calculating
PreSelectedInputs::total_amount when PreSelectedInputs::Insert()
was called.
return optional to force callers to explicitly handle the case
when effective amount optional is not set.
coins.size() would be the number of the OutputType keys in the map.
whereas Size() would return total number of COutput objects when
flattening the map.
before #25806, set<COutput> was used and would not
contain same COutputs in the set.
now we use set<shared_ptr<COutput>> and it might be
possible for 2 distinct shared_ptr (different pointer
address but same COutputs) to be added into the set.
so preserve previous behaviour by making sure values
in the set are also distinct
e67a676df9 fix: uptime RPC returns 0 on first call (Lőrinc)
Pull request description:
### Problem
#34328 switched uptime to use monotonic time, but `g_startup_time` was a function-local static in `GetUptime()`, meaning it was initialized on first call rather than at program start.
This caused the first uptime RPC to always return 0.
### Fix
Move `g_startup_time` to namespace scope so it initializes at program start, ensuring the first `uptime()` call returns actual elapsed time.
### Reproducer
Revert the fix and run the test or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
./build/bin/bitcoind -regtest -daemon
sleep 10
./build/bin/bitcoin-cli -regtest uptime
./build/bin/bitcoin-cli -regtest stop
```
<details>
<summary>Before (uptime is initialized on first call)</summary>
```bash
Bitcoin Core starting
0
Bitcoin Core stopping
```
</details>
<details>
<summary>After (first uptime call is in-line with sleep)</summary>
```bash
Bitcoin Core starting
10
Bitcoin Core stopping
```
</details>
----
Fixes#34423, added reporter as coauthor.
ACKs for top commit:
maflcko:
lgtm ACK e67a676df9
optout21:
crACK e67a676df9
carloantinarella:
Tested ACK e67a676df9
achow101:
ACK e67a676df9
musaHaruna:
Tested ACK [e67a676](e67a676df9)
Tree-SHA512: b156f7ed15c3fbb50e8a15f6c9a0d4e2ffb956d0c6ef949e0f8a661a564a20c0d3ed2149fae75ce7e2baa9326788d5037e726e7a7ac2c6ef4e70e4862cd5763f
54d0393058 FUZZ: Test that BnB finds best solution (Murch)
Pull request description:
BnB’s solution is the input set with the lowest waste score, excluding any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to ensure that BnB succeeds.
ACKs for top commit:
achow101:
ACK 54d0393058
brunoerg:
ACK 54d0393058
Tree-SHA512: 96b6a822f53311d9a76abe8c217794e0a2dd5bd713db0a15dc70e065099b8245c430e1174e24133e0a802218ff0f2943dfcc3d638c3716485d5607c452854e7d
322c4ec442 build: replace WERROR with CMAKE_COMPILE_WARNING_AS_ERROR (fanquake)
Pull request description:
`CMAKE_COMPILE_WARNING_AS_ERROR` was added to CMake in 3.24.
`--compile-no-warning-as-error` can be used, if needed, in future, to
suppress the `CMAKE_COMPILE_WARNING_AS_ERROR` behaviour from a CI
config.
Potential alternative to #33297. Closes#33284.
See https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html.
ACKs for top commit:
purpleKarrot:
re-ACK 322c4ec442
maflcko:
review ACK 322c4ec442📩
hebasto:
re-ACK 322c4ec442.
Tree-SHA512: 1e9b41e87568c1021aecf9414cfc70a3215d1663a13adb1f32493f2dba18978b53888520775d1aa2945224b0404ef83bda0837dddf5f8a089cafb940b856598c
f2b8acc0ed remove glozow from trusted keys (glozow)
Pull request description:
As planned!
Also bump trusted-git-root to 88a7294356, which is the last merge signed with this key.
ACKs for top commit:
achow101:
ACK f2b8acc0ed
stickies-v:
ACK f2b8acc0ed
Tree-SHA512: e68025c519ab003ad464783325502c1a6a3f560c0708a9383332cb0971be2039f462fc95f85373bb075191c165ca8e3904312a66e41b52d1b99dd2a8a837be9a
-Werror is added to the previous releases job, given it runs on Ubuntu
22.04, which uses an older CMake.
`--compile-no-warning-as-error` can be used, if needed, in future, to
suppress the `CMAKE_COMPILE_WARNING_AS_ERROR` behaviour from a CI
config.
CMAKE_COMPILE_WARNING_AS_ERROR was added to CMake in 3.24.
See https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html.
Co-authored-by: willcl-ark <will8clark@gmail.com>
a50d0b6720 build: don't pass on boost dependency to kernel consumers (Cory Fields)
Pull request description:
This is unnecessary now that the kernel now exports a (boost-less) API.
Noticed while slimming down boost dependencies in #34495.
ACKs for top commit:
stickies-v:
ACK a50d0b6720
hebasto:
ACK a50d0b6720, I have reviewed the code and it looks OK. I tested it by applying the Boost-specifc commits from https://github.com/bitcoin/bitcoin/pull/34143 and building with depends.
Tree-SHA512: e2d12356f41dd51dd729362121a33bd4f395821d53dd9a0bb0d5d6a53aba2ca2064e0709d9799dc6751b3d61ea576d2efc0e28296fdba26f2809dbcb0feabe44
1f8f7d477a Change BlockRequestAllowed() to take ref (optout)
Pull request description:
As [suggested here](https://github.com/bitcoin/bitcoin/pull/34416#discussion_r2745302958), a minor refactor of `PeerManagerImpl::BlockRequestAllowed()` to take reference parameter (instead of pointer). The motivation is to make the code safer, by minimizing the risk of null-dereference, and to be more consistent.
The change is local to the `PeerManagerImpl::BlockRequestAllowed()` class.
Related to #34440.
ACKs for top commit:
maflcko:
review ACK 1f8f7d477a🎐
l0rinc:
tested ACK 1f8f7d477a
stickies-v:
ACK 1f8f7d477a
Tree-SHA512: 9c2de2d3e7d067e018db7ec54c79f512ccc1da54574d4fb362f6697ee6e235783779d7094cf20856cd34e08a1dbc74609d8351fe7b2287cd8ec0c836ef07be19
42ee31e80c doc: fix broken bpftrace installation link (jayvaliya)
Pull request description:
The bpftrace project has moved from the `iovisor` organization to its own
`bpftrace` organization on GitHub. The old installation documentation link
(`https://github.com/iovisor/bpftrace/blob/master/INSTALL.md`) is now broken (404).
The project also restructured their documentation - installation instructions
are now in the README.md under the "Quick Start" section rather than a
separate INSTALL.md file.
ACKs for top commit:
maflcko:
lgtm ACK 42ee31e80c
Tree-SHA512: 5320e747b77fe9bb347a98396364f3942214b6d3ee69707f62452df0aaaa2f6cfd58fc3b6155f64c4acb00f1a4dba8a751618805e75b823454c73ac93877ad4f
The bpftrace project moved from iovisor/bpftrace to bpftrace/bpftraceand
removed the separate INSTALL.md file. Installation instructionsare now
in the README.md Quick Start section.
BnB’s solution is the input set with the lowest waste score, excluding
any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to
ensure that BnB succeeds.
db2effaca4 scripted-diff: refactor: CWallet::Create() -> CreateNew() (David Gumberg)
27e021ebc0 wallet: Correctly log stats for encrypted messages. (David Gumberg)
d8bec61be2 wallet: remove loading logic from CWallet::Create (David Gumberg)
f35acc893f refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` (David Gumberg)
e12ff8aca0 test: wallet: Split create and load (David Gumberg)
70dbc79b09 wallet: Use CWallet::LoadExisting() for loading existing wallets. (David Gumberg)
ae66e01164 wallet: Create separate function for wallet load (David Gumberg)
bc69070416 refactor: Wallet stats logging in its own function (David Gumberg)
a9d64cd49c wallet: Remove redundant birth time update (David Gumberg)
b4a49cc727 wallet: Move argument parsing to before DB load (David Gumberg)
b15a94a618 refactor: Split out wallet argument loading (David Gumberg)
a02c4a82d8 refactor: Move -walletbroadcast setting init (David Gumberg)
411caf7281 wallet: refactor: PopulateWalletFromDB use switch statement. (David Gumberg)
a48e23f566 refactor: wallet: move error handling to PopulateWalletFromDB() (David Gumberg)
0972785fd7 wallet: Delete unnecessary PopulateWalletFromDB() calls (David Gumberg)
f0a046094e scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB (David Gumberg)
Pull request description:
This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in `CWallet::Create()` into `CWallet::CreateNew()` and `CWallet::LoadExisting()`
The real win of this PR is that `CWallet::Create()` uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:
370c592612/src/wallet/wallet.cpp (L2882-L2885)
This heuristic assumes that wallets with no `ScriptPubKeyMans` are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.
It was already the case that every caller of `CWallet::Create()` knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.
ACKs for top commit:
achow101:
ACK db2effaca4
polespinasa:
approach ACK db2effaca4
w0xlt:
reACK db2effaca4
murchandamus:
ACK db2effaca4
rkrux:
ACK db2effaca4
Tree-SHA512: c28d60e0a3001058da3fd2bdbe0726c7ebe742a4b900a1dee2e5132eccc22e49619cb747a99b4032b000eafd4aa2fdd4ec244c32be2012aba809fdc94b5f6ecd
fad9dd1a88 test: kernel test fixups (MarcoFalke)
fabb58d42d test: Use clang-tidy named args for create_chainman (MarcoFalke)
fa51594c5c refactor: Small style fixups in src/kernel/bitcoinkernel.cpp (MarcoFalke)
Pull request description:
Just some small style and test fixups after https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3420542946
ACKs for top commit:
stickies-v:
re-ACK fad9dd1a88
frankomosh:
Code Review ACK fad9dd1a88. All changes are sound refactoring with no functional issues. Nice improvements to readability (named args in create_chainman, span.data(), range checks now properly require non-empty).
Tree-SHA512: 0a92e871b4db75a590acad39672594625e402895bc0d36635d36ec2fe8dce7cc2c5cb6ebf2a92bc14617d94648b84bffb95ff783cea71bd91ac4a9871ef5dbef
fa33acec89 Revert "valgrind: add suppression for bug 472219" (MarcoFalke)
Pull request description:
Now that several Alpine releases are out, it seems fine to remove this suppression.
This should work since Alpine 3.19: https://pkgs.alpinelinux.org/packages?name=valgrind&branch=v3.19&repo=&arch=aarch64, which just dropped out of `main` support (https://alpinelinux.org/releases/)
ACKs for top commit:
fanquake:
ACK fa33acec89
Tree-SHA512: 2a58475d20f8214c012b77efb7fe145e810a1506736341f8debea74dabd31499899a968bc2d4a88008b2a3878b4fe5e48fa14c83a2e943a91943bf0e1e159b6f
4dfb6eef70 test: Add DERSIG tests to script_tests (billymcbip)
884978f389 test: Fix a STRICTENC test in script_tests (billymcbip)
527e8ca7b5 test: Remove outdated comment in script_tests (billymcbip)
Pull request description:
1. Remove a comment referencing a file that no longer exists in the codebase: `script_invalid.json`.
2. Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in `CHECKMULTISIG`. Use `OP_1` for the second signature instead of `OP_0`.
3. Copy existing `STRICTENC` tests and change the flag to `DERSIG`. `DERSIG` is a consensus flag (unlike `STRICTENC`), so it'd be good to have dedicated test cases.
`script_tests` pass on my end.
ACKs for top commit:
darosior:
reACK 4dfb6eef70
sipa:
ACK 4dfb6eef70
Tree-SHA512: aea4aa5199530804561e9f597f69d6cffd7a40d4830919f9371fe97e4d04d8f10e8ed29b91e65e007a3f6e3a38e2881f88dff25831e741ad7592a12ed02b801a
The goal of the patch is to suggest C++ headers rather than their C
counterparts. However, for fixed width integer types, the patched IWYU
currently suggests `<cinttypes>` where `<cstdint>` is sufficient.
This change fixes this behavior.
02b5f6078d fees: make flushes log debug only (ismaelsadeeq)
Pull request description:
This is a simple PR that updates the flushing log to use debug-level logging under the estimatefee category. It also ensures the log consistently includes only the full file path.
The motivation behind this is that the "Flushed fee estimates to fee_estimates.dat." logs can become noisy; it's done after one hour, so hiding it in the debug estimatefee category seems reasonable.
---
I left the logs when the file is not found as info because that should only occur when you start a fresh node, change datadir, or explicitly delete the file
ACKs for top commit:
achow101:
ACK 02b5f6078d
furszy:
utACK 02b5f6078d
l0rinc:
Lightly tested ACK 02b5f6078d
sipa:
utACK 02b5f6078d
Tree-SHA512: 3e4b822caa23db9b30f1bb8990b36f35dcfcd82dbfb27c319463447da1df988eded84c47d5319ad637c822bdd04f0a727a176da3632c40d786332b6a5aaa6d89
