💬 achow101 commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2573979121)
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab

💬 mzumsande commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904669954)
Just a drive-by comment, but there is a typo in the PR title (revelant -> relevant)

💬 achow101 commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#issuecomment-2573988301)
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40

💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#issuecomment-2573990548)
There's still a [typo](https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901981925) there, could we fix it before merging? Otherwise I'd ACK as well.

💬 achow101 commented on pull request "contrib: fix BUILDDIR in gen-bitcoin-conf script":
(https://github.com/bitcoin/bitcoin/pull/31332#issuecomment-2573992956)
> It's not clear for me what is the desired way forward.

I have no preference either way, but I think the only reasonable options:
1. Close the PR
2. Update both the docs and script

As it is now, the docs and script are consistent with each other in master, but conflicting in this PR.

💬 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.