💬 achow101 commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1904685576)
nit: The style guide says to use the new style for new code, regardless of the style of the surrounding code. So this should use `snake_case` with `m_` prefix: `m_feerate_histogram`.

💬 achow101 commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1904686779)
nit: This should be named similarly to the txid to avoid confusion that this might be a different tx altogether. I suggest either calling this `parent_tx` or renaming `hashParentTx` to `hash_low_fee_tx`.

Also `snake_case` here and elsewhere in this test.

💬 davidgumberg commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1904731564)
just an observation: in all branches other than this one `children.empty() == true` since `node.subs.empty() == true`, and more generally `children.size() == node.subs.size()`

Verified that this is the case with the following diff:

```diff
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
@@ -527,6 +527,7 @@ struct Node {
{
// Use TreeEval() to avoid a stack-overflow due to recursion
auto upfn = [](const Node& node, Span<NodeRef<Key>> children) {
+
...

🚀 achow101 merged a pull request: "test: have miner_tests use Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31581)

💬 davidgumberg commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2574104916)
crACK https://github.com/bitcoin/bitcoin/pull/30866/commits/815467f46f47df6ff52686b0144430c14a31f4a8

This resolves #30864.

```console
$ echo "dHIoJTE3LzwyOzM+LGw6cGsoJTA4KSk=" | base64 --decode > scriptpubkeyman.crash
$ FUZZ=scriptpubkeyman ./build/src/test/fuzz/fuzz scriptpubkeyman.crash
scriptpubkeyman: succeeded against 1 files in 0s.
```

The default copy constructor of `miniscript::Node` can't be safe because of the custom iterative destructor `miniscript::~Node` and deleting it
...

💬 brunoerg commented on issue "failure in wallet_sendall.py --descriptors":
(https://github.com/bitcoin/bitcoin/issues/31571#issuecomment-2574106418)
I can't reproduce with the provided steps on Ubuntu 22.04 as well. Just tried it on MacOS 14.3 and worked.

```sh
...
wallet_resendwallettransactions.py --descriptors | ✓ Passed | 3 s
wallet_send.py --descriptors | ✓ Passed | 16 s
wallet_sendall.py --descriptors | ✓ Passed | 2 s
wallet_sendmany.py --descriptors | ✓ Passed | 1 s
wallet_signer.py --descriptors | ✓ Passed | 3
...

🚀 achow101 merged a pull request: "doc: Clarify comments about endianness after #30526"
(https://github.com/bitcoin/bitcoin/pull/31596)

💬 achow101 commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#issuecomment-2574150104)
@davidgumberg Wrote a test for a different PR that is failing for a similar issue, but in miniscript inferring. I've pushed that test and a commit to fix it as it's also a parity bit issue.

💬 theStack commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1904770235)
Fixed.

💬 theStack commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1904770569)
Thanks, moved the download instructions directly into the assertion message as suggested.

💬 theStack commented on pull request "test: raise an error in `_bulk_tx_` when `target_vsize` is too low":
(https://github.com/bitcoin/bitcoin/pull/30322#issuecomment-2574187864)
Concept ACK, thanks for following up!

LGTM, but would prefer to keep the vsize check in the MiniWallet functional framework test (first commit). It's true that this is currently unreachable, but a potential bug where e.g. the logic would change in a way that `_bulk_tx` isn't even called would then remain undetected by the test.

👍 pablomartin4btc approved a pull request: "wallet: migration, avoid loading legacy wallet after failure when BDB isn't compiled"
(https://github.com/bitcoin/bitcoin/pull/31451#pullrequestreview-2533263278)
tACK 589ed1a8eafe1daed379ebb383602c8f220aef19

I've reproduced the issue described in #31447 (sqllite-only or BDB not compiled). This PR fixed the test issue and it temporarily adds (later will be removed with BDB) a validation when the legacy wallet is loaded (with BDB actually compiled; `cmake -B build -DWITH_BDB=ON`).

🤔 ryanofsky reviewed a pull request: "doc: Clarify comments about endianness after #30526"
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2533252948)
Thanks for the reviews. Since there are more followups that could be made here, I created a [branch](https://github.com/ryanofsky/bitcoin/commits/pr/docblob2) to keep track of them. I don't want to create too much churn with documentation PRs so I won't open another one right away, but I can implement any suggestions made here, open a pr if requested, or wait for a related PR to be opened to attach these changes to.

💬 ryanofsky commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1904820053)
re: https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1903895648

> Nit: Call it `hexadecimal` instead of `hex` like done in the `"data"` field for better consistency?

I would probably prefer to go the other way since the term "hex" occurs more frequently than "hexadecimal" in RPC code and documentation (115 vs 11 times). I'm hoping we might be able to improve consistency by moving this information from descriptions of RPC fields to descriptions of RPC types and left another comment
...

💬 ryanofsky commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1904818269)
re: https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901981925

Thanks, fixed in a followup branch.

💬 ryanofsky commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1904820403)
re: https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901655760

> Could we unify the rest as well

To follow up, I looked at occurrences of `"txid"`, `"wtxid"`, and `"hash"` (quoted strings) in the RPC code and found dozens of fields that could be updated to be consistent. It seems more challenging than I expected to update all the documentation, and now I'm also wondering if might actually be harmful to mention reversed bytes in some cases but not others, because this might give t
...

💬 starius commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2574305884)
@achow101 I tried the updated version (2da3f0e659d3e89da0cdf525f8ce370bb35365a1).
I tested GUI flow - it works the same (doesn't produce a transaction).
I also re-tested `walletprocesspsbt` flow - now it is also broken:

```
walletprocesspsbt "cHNidP8BAH4CAAAAAfkoG4WU8+OG7ihR9ax1V+NQK6C9ZIEbsNH8qfB/A90YAAAAAAD9////AlcDAAAAAAAAF6kUp6q1daWOXVcRwue0FRtYgEAxvTCHKCMAAAAAAAAiUSBug0N9qhtsEJizov7RUbZtdDokLCvlo5+zNl+ocwJn636BAwAAAQErECcAAAAAAAAiUSBZP7q1V48G5XegaVSU+plRyc0hddLNxEwKjgaxGnEVXwEDBAEAAAA
...

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2574390155)
> @achow101 I tried the updated version ([2da3f0e](https://github.com/bitcoin/bitcoin/commit/2da3f0e659d3e89da0cdf525f8ce370bb35365a1)). I tested GUI flow - it works the same (doesn't produce a transaction). I also re-tested `walletprocesspsbt` flow - now it is also broken:

Ah, the sighash stuff needs a bit more work, and it also has impacts outside of MuSig support.

For the GUI workflow, if you apply https://github.com/bitcoin-core/gui/pull/850 on top of 3649c2e, it should "work". However
...

💬 vasild commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2574425798)
@maflcko, good question. I guess the answer is "yes". @ldionne, @var-const is it indeed the case that if libc++ itself is compiled with `_LIBCPP_ABI_BOUNDED_*`, then the user programs that link against such a libc++ must be compiled with `_LIBCPP_ABI_BOUNDED_*` as well?

In this PR we have a libc++ with and a user program without `_LIBCPP_ABI_BOUNDED_*` and it results in:
```
undefined reference to `std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::insert
...

💬 luke-jr commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2574452128)
Can we make the exists/is_fifo/open atomic somehow? Seems liable to have a race here someday...