📝 achow101 opened a pull request: "gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs"
(https://github.com/bitcoin-core/gui/pull/850)
SIGHASH_DEFAULT should be used to indicate SIGHASH_DEFAULT for taproot inputs, and SIGHASH_ALL for all other input types. This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.

See also #22514

💬 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-2573944915)
> Succeeded using `walletprocesspsbt`, but failed when using GUI "Load PSBT from keyboard" option.

Thanks for testing! This revealed an issue with sighash type handling in the aggregation code. I've pushed a fix for it, as well as a functional test which could replicate the issue with `walletprocesspsbt`.

This fix does require changing how we handle non-default sighash types - namely we now will add `PSBT_IN_SIGHASH` to an input if we are trying to sign it with something other than SIGHASH
...

💬 mzumsande commented on issue "assumevalid is not always applied when reindexing":
(https://github.com/bitcoin/bitcoin/issues/31494#issuecomment-2573967744)
> I added a test reproducing this bug https://github.com/Eunovo/bitcoin/commit/5e1ff5fe2c7374f97eb56454099c1b235c75e23c

Thanks, nice test (which should fail on master if I understand it correctly)!

> Isn't it still valuable to check that the current block is an ancestor of the most work chain?

Keeping the check doesn't hurt, but I think in the situation of a reindex this should automatically be the case:

Outside of a reindex, we could have additional headers received via p2p for whic
...

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