Create a fee_rate (sat/vB) RPC param and replace overloading the conf_target and
estimate_mode params in the following 6 RPCs with it:
- sendtoaddress
- sendmany
- send
- fundrawtransaction
- walletcreatefundedpsbt
- bumpfee
In RPC bumpfee, the previously existing fee_rate remains but the unit is changed
from BTC/kvB to sat/vB. This is a breaking change, but it should not be an
overly risky one, as the units change by a factor of 1e5 and any fees specified
in BTC/kvB after this commit will either be too low and raise an error or be 1
sat/vB and can be RBFed.
Update the test coverage for each RPC.
Co-authored-by: Murch <murch@murch.one>
ad5cef5dfd doc: Update data directory path comments (Hennadii Stepanov)
b19e88230f util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)
Pull request description:
Wallet names in `listwalletdir` RPC are correct now, even if the `-datadir` path has any number of trailing `/`.
This PR is an alternative to #19933.
Fixes#19928.
ACKs for top commit:
MarcoFalke:
review ACK ad5cef5dfd🔙
promag:
Code review ACK ad5cef5dfd.
meshcollider:
Code review + test run ACK ad5cef5dfd
Tree-SHA512: bccabbd6c18243d48d15b2b27201cc0f5984623dcbc635c8740cf74523f359844c36eadd40391142874fcf452a43880bb6afbf89815ae736e499f9a98143a661
fa48405ef8 Warn on unknown rw_settings (MarcoFalke)
Pull request description:
Log a warning to debug log if unknown settings are encountered. This should probably only ever happen when the software is upgraded.
Something similar is already done for the command line and config file. See:
* test: Add test for unknown args #16234 (commit fa7dd88b71)
ACKs for top commit:
ryanofsky:
Code review ACK fa48405ef8. Looks good and I could see this being helpful for debugging. Thanks for taking suggestions
Tree-SHA512: cec7d88adf84fa0a842f56b26245157736eb50df433db951e622ea07fd145b899822b24cdab1d8b36c066415ce4f0ef09b493fa8a8d691532822a59c573aafa7
d103484fe8 util: Do not use gArgs global in ArgsManager member functions (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
practicalswift:
ACK d103484fe8: patch looks correct
Tree-SHA512: dda7a5062363170c6995f2fd8fda48c0a919e5ca67be9faa8f0fa66f9d3b535f134eb6f4860a0859bc5457c02230b34a8d1264045f22bed8d30668158ac2271f
-BEGIN VERIFY SCRIPT-
sed -i 's/Fee exceeds maximum configured by \-\maxtxfee/Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)/g' src/util/error.cpp test/functional/rpc_fundrawtransaction.py test/functional/rpc_psbt.py test/functional/rpc_rawtransaction.py test/functional/wallet_bumpfee.py test/functional/wallet_create_tx.py
-END VERIFY SCRIPT-
7be6ff6187 net: recognize TORv3/I2P/CJDNS networks (Vasil Dimov)
e0d73573a3 net: CNetAddr: add support to (un)serialize as ADDRv2 (Vasil Dimov)
fe42411b4b test: move HasReason so it can be reused (Vasil Dimov)
d2bb681f96 util: move HasPrefix() so it can be reused (Vasil Dimov)
Pull request description:
(chopped off from #19031 to ease review)
Add an optional support to serialize/unserialize `CNetAddr` in ADDRv2 format (BIP155). The new serialization is engaged by ORing a flag into the stream version.
So far this is only used in tests to ensure the new code works as expected.
ACKs for top commit:
Sjors:
re-tACK 7be6ff6187
sipa:
re-utACK 7be6ff6187
eriknylund:
ACK 7be6ff6187 I built the PR on macOS Catalina 10.15.6, ran both tests and functional tests. I've reviewed the code and think the changes look good and according to BIP155. I verified that the added Base32 encoding test looks as proposed and working. I've run a node for a week only with Onion addresses `-onlynet=onion` without issues and I can connect to other peer reviewers running TorV3 on their nodes and I can connect both of my test nodes to each other.
jonatack:
re-ACK 7be6ff6187 per `git diff b9c46e0 7be6ff6`, debug build, ran/running bitcoind with this change and observed the log and `-netinfo` peer connections while connected as a tor v2 service to both tor v2 peers and also five tor v3 peers.
hebasto:
ACK 7be6ff6187, tested on Linux Mint 20 (x86_64): on top of this pull and #19031 I'm able to connect to onion v3 addresses, and jonatack is able to connect to my created onion v3 address.
Tree-SHA512: dc621411ac4393993aa3ccad10991717ec5f9f2643cae46a24a89802df0a33d6042994fc8ff2f0f397a3dbcd1c0e58fe4724305a2f9eb64d9342c3bdf784d9be
Recognizing addresses from those networks allows us to accept and gossip
them, even though we don't know how to connect to them (yet).
Co-authored-by: eriknylund <erik@daychanged.com>
c4b85ba704 Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB (Luke Dashjr)
Pull request description:
Fixes a bug introduced in #19614
The LevelDB-specific fdatasync check was only using `AC_SUBST`, which works for Makefiles, but doesn't define anything for C++. Furthermore, the #define is typically 0 or 1, never undefined.
This fixes both issues by defining it and checking its value instead of whether it is merely defined.
Pulled out of #14501 by fanquake's request
ACKs for top commit:
fanquake:
ACK c4b85ba704 - thanks for catching and fixing my mistake.
laanwj:
Code review ACK c4b85ba704
Tree-SHA512: 91d5d426ba000b4f3ee7e2315635e24bbb23ceff16269ddf4f65a63d25fc9e9cf94a3b236eed2f8031cc36ddcf78aeb5916efcb244f415943a8a12f907ede8f9
102867c587 net: change CNetAddr::ip to have flexible size (Vasil Dimov)
1ea57ad674 net: don't accept non-left-contiguous netmasks (Vasil Dimov)
Pull request description:
(chopped off from #19031 to ease review)
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Do not accept
invalid netmasks that have 0-bits followed by 1-bits and only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
ACKs for top commit:
sipa:
utACK 102867c587
MarcoFalke:
Concept ACK 102867c587
ryanofsky:
Code review ACK 102867c587. Just many suggested updates since last review. Thanks for following up on everything!
jonatack:
re-ACK 102867c587 diff review, code review, build/tests/running bitcoind with ipv4/ipv6/onion peers
kallewoof:
ACK 102867c587
Tree-SHA512: d60bf716cecf8d3e8146d2f90f897ebe956befb16f711a24cfe680024c5afc758fb9e4a0a22066b42f7630d52cf916318bedbcbc069ae07092d5250a11e8f762
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
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
