8531e1e7312efe66204dc8ce56f21e44de99e956 ci: Use DEBUG=1 in depends for MSAN jobs (fanquake)
800ddef6b994b8b05a11f2c1c6d265daaec5751a ci: use LLVM 17.0.6 in MSAN jobs (fanquake)
Pull request description:
Switch to using LLVM 17.0.6 and `DEBUG=1` in MSAN CI jobs.
ACKs for top commit:
maflcko:
lgtm ACK 8531e1e7312efe66204dc8ce56f21e44de99e956
Tree-SHA512: 819889762aeb78f95c4f955978890c6d98884bed0c7ff97ec072f4c7c1119ee3e3268ccab795bb1c801d36a206e16c6c1195e7a2bc7af94b580d17e49c632161
fab97d81ce3740509dbbe9270ca67a1b65b00c72 fuzz: Print coverage summary after run_once (MarcoFalke)
Pull request description:
This can be used to quickly check the coverage effects of a code change or qa-assets change.
ACKs for top commit:
dergoegge:
ACK fab97d81ce3740509dbbe9270ca67a1b65b00c72
Tree-SHA512: 0ac913c14698f39e76e0e7bf124f182220031796d6443edb34c6e4615e128157cf746da661b216c4640a41964e977249712445ca9c005b1b4a3737adabdb4a7d
5b9d5bf866506b22270598aa2dc1269bc02e38e2 depends: remove (darwin) libtool now that it's no longer used (Cory Fields)
3ef6563495428d605409089b2267e18f2bf491f9 depends: use ar rather than libtool for miniupnpc/libnatpmp (Cory Fields)
Pull request description:
An alternative to https://github.com/bitcoin/bitcoin/pull/29232
Rather than switching to the CMake builds which [proved problematic](https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1898513919), do the quick and dirty thing of just patching out libtool. Doesn't seem to introduce any new issues.
This should buy us time to upstream the necessary CMake fixes.
ACKs for top commit:
TheCharlatan:
ACK 5b9d5bf866506b22270598aa2dc1269bc02e38e2
fanquake:
ACK 5b9d5bf866506b22270598aa2dc1269bc02e38e2
Tree-SHA512: c75c4bcc9332d8c1fc3395e2b5fc7265849186afc7005700f662ab291e6ea1f111025fad733d0b0b39d35029d1b757d3f1937d63aad3c0c3b88d0f8ac902ee18
bbf218d06164b7247f5e9df5ba143383022fbf74 crypto: remove sha256_sse4 from the base crypto helper lib (Cory Fields)
4dbd0475d8c16ed10c309bf6badc17f2d2eaaa69 crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 (Cory Fields)
Pull request description:
Replace it with a more explicit `DISABLE_OPTIMIZED_SHA256` and clean up some.
The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity.
Also remove the `BUILD_BITCOIN_INTERNAL` define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports.
Removing the define should have the effect of enabling sha256 optimizations for the kernel.
ACKs for top commit:
TheCharlatan:
Re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74
hebasto:
re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74
Tree-SHA512: 7c17592bb2d3e671779f96903cb36887c5785408213bffbda1ae37b66e6bcfaffaefd0c1bf2d1a407060cd377e3d4881cde3a73c429a1aacb677f370314a066a
3bfc5bd36e38ca07306a41a3f56ea102ffe02b67 test: ensure output is large enough to pay for its fees (stickies-v)
Pull request description:
Fixes a (rare) intermittency issue in wallet_import_rescan.py
Since we [use](03752444cd/test/functional/wallet_import_rescan.py (L296)) `subtract_fee_from_outputs=[0]` in the `send` command, the output amount must at least be as large as the fee we're paying.
Example in CI: https://api.cirrus-ci.com/v1/task/6107972259020800/logs/ci.log
```
2024-01-18T22:16:12.383000Z TestFramework (INFO): Test that the mempool is rescanned as well if the rescan parameter is set to true
2024-01-18T22:16:20.187000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_import_rescan.py", line 292, in run_test
child = self.nodes[1].send(
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: The transaction amount is too small to pay the fee (-4)
```
Can be reproduced locally by forcing usage of the lowest possible value produced by `get_rand_amount()` ([thanks furszy](https://github.com/bitcoin/bitcoin/pull/29283#pullrequestreview-1832956095)):
<details>
<summary>git diff on 5f3a0574c4</summary>
```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 7f01d23941..925849d5c0 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -270,7 +270,7 @@ class ImportRescanTest(BitcoinTestFramework):
address_type=variant.address_type.value,
))
variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
- variant.initial_amount = get_rand_amount() * 2
+ variant.initial_amount = Decimal(str(round(AMOUNT_DUST, 8))) * 2
variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
variant.confirmation_height = 0
variant.timestamp = timestamp
```
</details>
ACKs for top commit:
achow101:
ACK 3bfc5bd36e38ca07306a41a3f56ea102ffe02b67
glozow:
utACK 3bfc5bd36e38ca07306a41a3f56ea102ffe02b67, didn't experience this issue but in theory a minimum of `AMOUNT_DUST` could be too low to pay the fees
furszy:
utACK 3bfc5bd36
Tree-SHA512: 821ab94a510772e90528b2cef368bbf70309d8fd1dcda53dce75dd1bf91622358e80fea4d9fc68249b9d598892306c66f6c843b4a6855a9f9a9175f7b41109c6
8023640a71a10ec54a6a8e6b95a29d07f7be218d qt: Avoid non-self-contained Windows header (Hennadii Stepanov)
Pull request description:
Using the `windows.h` header guarantees correctness regardless of the content of other headers.
For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio
Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.
Related to https://github.com/hebasto/bitcoin/pull/77.
ACKs for top commit:
theuni:
ACK 8023640a71a10ec54a6a8e6b95a29d07f7be218d. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.
Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c
cf937b2068dba167b6c7ea6f8ee9ba366803c3bb fuzz: also set MSAN_SYMBOLIZER_PATH (fanquake)
Pull request description:
Should resolve: https://github.com/bitcoin-core/qa-assets/issues/167.
ACKs for top commit:
dergoegge:
utACK cf937b2068dba167b6c7ea6f8ee9ba366803c3bb
Tree-SHA512: a7670b5054c2c9ec830db2a4dd4d78d8a0ee7d793a80d32942d78b5e459015344040fa9ce9d73f4f23cd920d5ca2e65c110e201723e4935de8f57fda0b6d5ce7
fa3373d3adbace7e4665cf391363319a55a09a96 refactor: Compile unreachable code (MarcoFalke)
Pull request description:
When unreachable code isn't compiled, compile failures are not detected.
Fix this by leaving it unreachable, but compiling it.
Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916
ACKs for top commit:
achow101:
ACK fa3373d3adbace7e4665cf391363319a55a09a96
ryanofsky:
Code review ACK fa3373d3adbace7e4665cf391363319a55a09a96. This looks good, and should prevent code in the else blocks from accidentally breaking.
Tree-SHA512: 3a3764915dfc935bf5d7a48f1ca151dcbac340c1cbdce8236b24ae9b4f04d6ee9771ed058ca60bcbca6e19d13671de3517f828a8f7ab6444c7cc4e3538d1ba4e
d298ff8b62b2624ed390c8a2f905c888ffc956ff During IBD, prune as much as possible until we get close to where we will eventually keep blocks (Luke Dashjr)
Pull request description:
This should reduce pruning flushes even more, speeding up IBD with pruning on systems that have a sufficient dbcache.
Assumes 1 MB per block between tip and best header chain. Simply adds this to the buffer pruning is trying to leave available, which results in pruning almost everything up until we get close to where we need to be keeping blocks.
ACKs for top commit:
andrewtoth:
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
fjahr:
utACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
achow101:
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
Tree-SHA512: 2a482376bfb177e2ba7c2f0bb0b58b02efdb38b34755a18d1fc3e869df5959c85b6f1009e1386fa8b89c4f90d520383e36bd3e21dec221042315134efb1a455b
5fb8f0f80fc41cc636da56864195244d8fd9116e depends: Do not override CFLAGS when building SQLite with DEBUG=1 (Hennadii Stepanov)
2b0dd88f1ce9084324dc54db578fade9c926fd71 depends: Ensure definitions are passed when building SQLite with DEBUG=1 (Hennadii Stepanov)
Pull request description:
The `--enable-debug` configure option for the SQLite package does two things:
```autoconf
#-----------------------------------------------------------------------
# --enable-debug
#
AC_ARG_ENABLE(debug, [AS_HELP_STRING(
[--enable-debug], [build with debugging features enabled [default=no]])],
[], [])
AC_MSG_CHECKING([Build type])
if test x"$enable_debug" = "xyes"; then
BUILD_CFLAGS="$BUILD_CFLAGS -DSQLITE_DEBUG -DSQLITE_ENABLE_SELECTTRACE -DSQLITE_ENABLE_WHERETRACE"
CFLAGS="-g -O0"
AC_MSG_RESULT([debug])
else
AC_MSG_RESULT([release])
fi
#-----------------------------------------------------------------------
```
It adds three preprocessor definitions and overrides `CFLAGS` with `"-g -O0"`. The latter breaks the user's ability to provide sanitizer and LTO flags.
This PR might be especially useful for OSS-Fuzz where `DEBUG=1` has been used since https://github.com/google/oss-fuzz/pull/10503.
Also it makes a workaround for building SQLite for 32-bit unneeded. For details, please refer to https://github.com/hebasto/oss-fuzz/tree/240120-sqlite.
Changes in https://github.com/bitcoin/bitcoin/pull/29282 might not be strictly required now. However, I consider them an improvement.
ACKs for top commit:
fanquake:
ACK 5fb8f0f80fc41cc636da56864195244d8fd9116e - downstream is also green, so i'll fixup the PR there.
Tree-SHA512: 8593d8a0237ebb270d5da763fb65ed642ab8ed0d44e57704a34154621f49e3d5c58b462cc0070251fa1ba556c58a3c7d3620530d6839dc6dc9e0887010330eca
The `--enable-debug` configure option for the SQLite package does two
things. It adds three preprocessor definitions and overrides CFLAGS with
"-g -O0". The latter breaks the user's ability to provide sanitizer and
LTO flags.
00c1e2aa4496b5f038ae5199dbd16d8313766533 build: fix optimisation flags used for --coverage (fanquake)
1dc2c9b385f8345c588449848149b8e470653afc ci: cleanup C*FLAG usage in Valgrind jobs (fanquake)
6cc2a38c1388b696e9c28a08c6bd9c93da4fa6b8 build: add sanitizer flags to configure output (fanquake)
08cd5aca18f0774258c7c459773b9e8b386d48ef build: always set -g -O2 in CORE_CXXFLAGS (fanquake)
Pull request description:
Rather than trying to sporadically rely on / override Autoconf default behaviour. Just always override (if unset), and always set the flags we want (which are the same as the Autoconf defaults).
Removes the need for duplicate code to clear (if not overridden) `CXXFLAGS`.
Fixes cases of "missing" `-O2`. i.e this PR when running a Valgrind CI job with changes here:
```bash
CXXFLAGS = -g -O2 -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -mbranch-protection=bti -Werror -fsanitize=fuzzer -gdwarf-4
```
Fixes configure output to reflect actual compilation flag ordering, so it's useful.
Note that if we do still end up with a duplicate "-g -O2" when compiling, that has no effect, and I don't really thinks it's something worth trying to optimize.
ACKs for top commit:
TheCharlatan:
lgtm ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533
hebasto:
ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533, I have reviewed the code and it looks OK. Also tested `ci/test/00_setup_env_native_valgrind.sh`.
theuni:
ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533
Tree-SHA512: cf6c7acf813ba10b198561e83eb72e9b2532a39cb1767c452d031e82921dcd42a47b129735b24c4e36131fd0c8fe7457f7cae870c1e011cdfdd430bdc4d4912b
ff54314d4abed3bf9a78daf785a01c63af15c69d wallet: clarify replaced_by_txid and replaces_txid in help output (marco)
Pull request description:
Resolves issue #27781
ACKs for top commit:
achow101:
ACK ff54314d4abed3bf9a78daf785a01c63af15c69d
ryanofsky:
Code review ACK ff54314d4abed3bf9a78daf785a01c63af15c69d. Seems like a helpful clarification
Tree-SHA512: b13a0e24505dfaee083467ac6f357b96460b5d1841dc29c4df4a503c290d379cef3d50fcc76f33bbc95741f484dd9d2461b0c2e8bdebf57a8a72edfbeece2a79
9d09c873a50d344e2a9cb35fe246a52688b9fa34 fuzz: Exit and log stderr for parse_test_list errors (dergoegge)
Pull request description:
We should log all errors that occur when attempting to print the harness list in the fuzz test runner.
ACKs for top commit:
maflcko:
lgtm ACK 9d09c873a50d344e2a9cb35fe246a52688b9fa34
Tree-SHA512: 50471b732c8cbe287dacba14487e7c8a5826f146432d93aa3bb55d063a8ba158d01641d6cb1360241dd4cd54ef5e045b0412f9cc34d06c181134921d1f1ceced
18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d refactor: pass CRecipient to FundTransaction (josibake)
5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 refactor: simplify `CreateRecipients` (josibake)
47353a608dc6e20e5fd2ca53850d6f9aa3240d4a refactor: remove out param from `ParseRecipients` (josibake)
f7384b921c3460c7a3cc7827a68b2c613bd98f8e refactor: move parsing to new function (josibake)
6f569ac903e5ddaac275996a5d0c31b2220b7b81 refactor: move normalization to new function (josibake)
435fe5cd96599c518e26efe444c9d94d1277996b test: add tests for fundrawtx and sendmany rpcs (josibake)
Pull request description:
## Motivation
The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`.
As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO.
I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments.
ACKs for top commit:
S3RK:
reACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
josibake:
> According to my `range-diff` nothing changed. reACK [18ad1b9](18ad1b9142)
achow101:
ACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
6acec6b9ff02b91de132bb1575d75908a8a2d27b multiprocess: Add type conversion code for UniValue types (Ryan Ofsky)
0cc74fce72e0c79849109ee5d7afe707991b3512 multiprocess: Add type conversion code for serializable types (Ryan Ofsky)
4aaee239211a5287fbc361c0eb158b105ae8c8db test: add ipc test to test multiprocess type conversion code (Ryan Ofsky)
Pull request description:
Add type conversion hooks to allow `UniValue` objects, and objects that have `CDataStream` `Serialize` and `Unserialize` methods to be used as arguments and return values in Cap'nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly.
The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 6acec6b9ff02b91de132bb1575d75908a8a2d27b
dergoegge:
reACK 6acec6b9ff02b91de132bb1575d75908a8a2d27b
Tree-SHA512: 5d2cbc5215d488b876d34420adf91205dabf09b736183dcc85aa86255e3804c2bac5bab6792dacd585ef99a1d92cf29c8afb3eb65e4d953abc7ffe41994340c6
e9014042a6bed8c16cc9a31fc35cb709d4b3c766 settings: add auto-generated warning msg for editing the file manually (furszy)
966f5de99a9f5da05c91378ad1e8ea8ed37ac3b3 init: improve corrupted/empty settings file error msg (furszy)
Pull request description:
Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144).
Improving a confusing situation reported by users who did not understand why a
settings parsing error occurred when the file was empty and did not know how to solve it.
Empty setting file could be due (1) corruption or (2) an user manually cleaning up the file content.
In both scenarios, the 'Unable to parse settings file' error does not help the user move forward.
ACKs for top commit:
achow101:
ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766
hebasto:
re-ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766.
ryanofsky:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766. Just whitespace formatting changes and shortening a test string literal since last review
shaavan:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766
Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov)
Pull request description:
`CWallet::GetEncryptionKey()` would return a reference to the internal
`CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.
Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after `CWallet::Lock()` (the current calls to
`CWallet::GetEncryptionKey()` are safe, but that is not future proof).
So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
change the `GetEncryptionKey()` method to provide the encryption
key to a given callback:
`m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`
This silences the following (clang 18):
```
wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
3520 | return vMasterKey;
| ^
```
---
_Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit 856c88776f was merged in https://github.com/bitcoin/bitcoin/pull/29040 so now this only affects wallet code. The previous PR description was:_
Avoid this unsafe pattern from `ArgsManager` and `CWallet`:
```cpp
class A
{
Mutex mutex;
Foo member GUARDED_BY(mutex);
const Foo& Get()
{
LOCK(mutex);
return member;
} // callers of `Get()` will have access to `member` without owning the mutex.
```
ACKs for top commit:
achow101:
ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b
ryanofsky:
Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple.
furszy:
ACK 32a9f13c
Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1
d55fdb1a495190e213b1b5127f5d91e4a409765e Move TRACEx parameters to seperate lines (Richard Myers)
2d58629ee63eebc760e2a9226afcd0c46d3ec2bd wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers)
Pull request description:
This is a bugfix for from when [optional was introduced](758501b713) for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0.
I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change.
You can reproduce this bug using the coin-selection-simulation scripts as described in [issue #16](https://github.com/achow101/coin-selection-simulation/issues/16). You can also run the `interface_usdt_coinselection.py` test without the changes to `wallet/spend.cpp`.
ACKs for top commit:
0xB10C:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
achow101:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
murchandamus:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
97181decf5726aab6c5cd01b3e1964072f2531ff Add test for negative transaction version w/ CSV to tx_valid.json (Chris Stewart)
Pull request description:
This PR adds a static test vector corresponding to the bug found in various implementations of the bitcoin protocol discovered by dergoegge
For more information see:
https://delvingbitcoin.org/t/disclosure-btcd-consensus-bugs-due-to-usage-of-signed-transaction-version/455
ACKs for top commit:
darosior:
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff
dergoegge:
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff
Tree-SHA512: 92bbcd3cd10a569757b4de91e1b2bcfebc2b75ddb0160be36d8e512a6fa4623cced1aba93bd1cc044962cd2b10e1d184ef109ccdfe3cfcf85cf4b9585d80d115
Hopefully, refraining users from modifying the file unless they are
certain about the potential consequences.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The preceding "Unable to parse settings file" message lacked
the necessary detail and guidance for users on what steps to
take next in order to resolve the startup error.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec refactor: remove CTxMemPool::queryHashes() (stickies-v)
Pull request description:
`CTxMemPool::queryHashes()` is only used in `MempoolToJSON()`, where it can just as easily be replaced with the more general `CTxMemPool::entryAll()`. No behaviour change, just cleans up the code.
ACKs for top commit:
dergoegge:
Code review ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec
TheCharlatan:
ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec
glozow:
ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec. Looks like there's no conflicts.
Tree-SHA512: 16160dec8e1f2457fa0f62dc96d2d2efd92c4bab810ecdb0e08918b8e85a667702c8e41421eeb4ea6abe92a5956a2a39a7a6368514973b78be0d22de2ad299b2
Fixes a (rare) intermittency issue in wallet_import_rescan.
Since we use `subtract_fee_from_outputs=[0]` in the `send` command,
the output amount must at least be as large as the fee we're paying.
Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction`
take a `CRecipient` vector directly. This allows us to remove SFFO logic from
the wrapper RPC `FundTransaction` since the `CRecipient` objects have already
been created with the correct SFFO values. This also allows us to remove
SFFO from both `FundTransaction` function signatures.
This sets us up in a future PR to be able to use these RPCs with BIP352
static payment codes.
Move validation logic out of `CreateRecipients` and instead take the
already validated outputs from `ParseOutputs` as an input.
Move SFFO parsing out of `CreateRecipients` into a new function,
`InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions
from `sendmany` and `sendtoaddress` and turns them into a set of integers.
In a later commit, we will also move the SFFO parsing logic from
`FundTransaction` into this function.
Worth noting: a user can pass duplicate addresses and addresses that dont exist
in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress`
without triggering a warning. This behavior is preserved in to keep this commit
strictly a refactor.
Move the parsing and validation out of `AddOutputs` into its own function,
`ParseOutputs`. This allows us to re-use this logic in `ParseRecipients` in a
later commit, where the code is currently duplicated.
The new `ParseOutputs` function returns a CTxDestination,CAmount tuples.
This allows the caller to then translate the validated outputs into
either CRecipients or CTxOuts.
Move the univalue formatting logic out of AddOutputs and into its own function,
`NormalizeOutputs`. This allows us to re-use this logic in later commits.
If the serialized transaction passed to `fundrawtransaction` contains
duplicates, they will be deserialized and added to the transaction. Add
a test to ensure this behavior is not changed during the refactor.
A user can pass any number of duplicated and unrelated addresses as an
SFFO argument to `sendmany` and the RPC will not throw an error (note,
all the rest of the RPCs which take SFFO as an argument will error if
the user passes duplicates or specifies outputs not present in the
transaction). Add a test to ensure this behavior is not changed during
the refactor.
6ec2813cd88d5f0b955d746e4711a8ad256ea47f depends: add NM output to gen_id (fanquake)
Pull request description:
`NM` is part of the current toolset, and can be set by the user. Include it in `gen_id`.
ACKs for top commit:
TheCharlatan:
Re-ACK 6ec2813cd88d5f0b955d746e4711a8ad256ea47f
Tree-SHA512: 2ada61e03783f9eb441f285ef5da50557ad729cb52ce2d2c4b2c38103dab29920a26262d4545fd2ac7fbf1cedc4902cd2359833544fbc0debf829c12a63e9769
