0a8aa626dd refactor: Make HexStr take a span (Wladimir J. van der Laan)
Pull request description:
Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses.
ACKs for top commit:
elichai:
Code review ACK 0a8aa626dd
hebasto:
re-ACK 0a8aa626dd
jonatack:
re-ACK 0a8aa626dd
Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992
1d8338d6b7 util: use HAVE_FDATASYNC to determine fdatasync() use (fanquake)
Pull request description:
Rather than just using on Linux and NetBSD, use `fdatasync()` based
on whether it's available. i.e `fdatasync` is available in newer versions of FreeBSD.
This also aligns more closely with what is being done in leveldb.
Was pointed out by Luke in #19430.
ACKs for top commit:
practicalswift:
ACK 1d8338d6b7 -- patch looks correct
laanwj:
ACK 1d8338d6b7
hebasto:
ACK 1d8338d6b7
Tree-SHA512: 7dd6d87f5dc0c0ba21ae42f96b63fc12b34806cd536457fc4284f14bb8c235765344be228b000c6adf4cd1e8c4e6a03a18ca18ab22599c42cc3b706e0bcd1a17
Rather than just using it on Linux and NetBSD, use `fdatasync()` based
on whether it's available. i.e it is available in newer versions
of FreeBSD (11.1 and later).
This also aligns our code more closely with what is being done in leveldb.
Was pointed out by Luke in #19430.
9c69cfe4c5 Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5700 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)
Pull request description:
Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.
ACKs for top commit:
MarcoFalke:
Approach re-ACK 9c69cfe4c5🌾
jnewbery:
utACK 9c69cfe4c5
Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
Persistent settings are used in followup PRs #15936 to unify gui settings
between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to
the loadwallet RPC and maintain a dynamic list of wallets that should be loaded
on startup that also can be shared between bitcoind and bitcoin-qt.
fa3365430c net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2f util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
fab80fef61 refactor: Remove unused EnsureChainman (MarcoFalke)
fa34587f1c scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke)
fa6ef701ad util: Add Assert identity function (MarcoFalke)
fa457fbd33 move-only: Move NDEBUG compile time check to util/check (MarcoFalke)
Pull request description:
The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated.
ACKs for top commit:
promag:
Tested ACK fab80fef61.
ryanofsky:
Code review ACK fab80fef61
Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
25dac9fa65 doc: add release notes for explicit fee estimators and bumpfee change (Karl-Johan Alm)
05227a3554 tests for bumpfee / estimate_modes (Karl-Johan Alm)
3404c1b753 policy: optional FeeEstimateMode param to CFeeRate::ToString (Karl-Johan Alm)
6fcf448430 rpc/wallet: add two explicit modes to estimate_mode (Karl-Johan Alm)
b188d80c2d MOVEONLY: Make FeeEstimateMode available to CFeeRate (Karl-Johan Alm)
5d1a411eb1 fees: add FeeModes doc helper function (Karl-Johan Alm)
91f6d2bc8f rpc/wallet: add conf_target as alias to confTarget in bumpfee (Karl-Johan Alm)
69158b41fc added CURRENCY_ATOM to express minimum indivisible unit (Karl-Johan Alm)
Pull request description:
This lets users pick their own fees when using `sendtoaddress`/`sendmany` if they prefer this over the estimators.
ACKs for top commit:
Sjors:
re-utACK 25dac9fa65: rebased, more fancy C++,
jonatack:
ACK 25dac9fa65 I think this should be merged after all this time, even though it looks to me like there are needed follow-ups, fixes and test coverage to be added (see further down), which I don't mind helping out with, if wanted.
fjahr:
Code review ACK 25dac9fa65
Tree-SHA512: f31177e6cabf3187a43cdfe93477144f8e8385c7344613743cbbd16e8490d53ff5144aec7b9de6c9a65eb855b55e0f99d7f164dee4b6bf3cfea4dce51cf11d33
The utility is primarily useful to dereference pointer types, which are
known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are
started. Instead of silently relying on that assumption, Assert can be
used to abort the program and avoid UB should the assumption ever be
violated.
- Move the decision whether to translate an error message to where it is
defined. This simplifies call sites: no more `InitError(Untranslated(...))`.
- Make all functions in `util/error.h` consistently return a
`bilingual_str`. We've decided to use this as error message type so
let's roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
Pthread library does not set errno.
Pthread library's errno is returned by return value.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Building with -Wunreachable-code-loop-increment causes a warning
due to always returning on the first iteration of the loop that
outputs errors on invalid args.
Collect all errors, and output them in a single error message
after the loop completes, resolving the warning and avoiding
popup hell by outputting a seperate message for each error.
5478d6c099 logging: thread safety annotations (Anthony Towns)
e685ca1992 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns)
a788789948 test/checkqueue_tests: thread safety annotations (Anthony Towns)
479c5846f7 rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns)
8b5af3d4c1 net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns)
de7c5f41ab wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns)
c3cf2f5501 rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns)
Pull request description:
In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes.
This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations.
It's based on top of #16112, and turns the thread safety comments included there into annotations.
It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well.
ACKs for top commit:
MarcoFalke:
ACK 5478d6c099🗾
hebasto:
re-ACK 5478d6c099, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review.
ryanofsky:
Code review ACK 5478d6c099. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard
Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
748977690e Add asmap_direct fuzzer that tests Interpreter directly (Pieter Wuille)
7cf97fda15 Make asmap Interpreter errors fatal and fuzz test it (Pieter Wuille)
c81aefc537 Add additional effiency checks to sanity checker (Pieter Wuille)
fffd8dca2d Add asmap sanity checker (Pieter Wuille)
5feefbe6e7 Improve asmap Interpret checks and document failures (Pieter Wuille)
2b3dbfa5a6 Deal with decoding failures explicitly in asmap Interpret (Pieter Wuille)
1479007a33 Introduce Instruction enum in asmap (Pieter Wuille)
Pull request description:
This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow.
In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file.
I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it.
ACKs for top commit:
practicalswift:
ACK 748977690e modulo feedback below.
jonatack:
ACK 748977690e code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight.
fjahr:
ACK 748977690e
Tree-SHA512: d876df3859735795c857c83e7155ba6851ce839bdfa10c18ce2698022cc493ce024b5578c1828e2a94bcdf2552c2f46c392a251ed086691b41959e62a6970821
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.
182dbdf0f4 util: Detect posix_fallocate() instead of assuming (Vasil Dimov)
Pull request description:
Don't assume that `posix_fallocate()` is available on Linux and not
available on other operating systems. At least FreeBSD has it and we
are not using it.
Properly check whether `posix_fallocate()` is present and use it if it
is.
ACKs for top commit:
laanwj:
ACK 182dbdf0f4
Tree-SHA512: f9ed4bd661f33ff6b2b1150591e860b3c1f44e12b87c35e870d06a7013c4e841ed2bf17b41ad6b18fe471b0b23a4b5e42cf1400637180888e0bc56c254fe0766
fa4632c417 test: Move boost/stdlib includes last (MarcoFalke)
fa488f131f scripted-diff: Bump copyright headers (MarcoFalke)
fac5c37300 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c417
jonatack:
ACK fa4632c417, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
Don't assume that `posix_fallocate()` is available on Linux and not
available on other operating systems. At least FreeBSD has it and we
are not using it.
Properly check whether `posix_fallocate()` is present and use it if it
is.
Don't require urlDecode function in wallet code since urlDecode implementation
currently uses libevent. Just call urlDecode indirectly though URL_DECODE
function pointer constant if available.
In bitcoind and bitcoin-qt, URL_DECODE is implemented and used to interpret RPC
wallet requests. In bitcoin-wallet, URL_DECODE is null to avoid depending on
libevent.
