💬 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-2573821002)
> commit message / PR description micro-nit: haven't seen the term "evenness" before in this context, I think we usually call it "parity bit" (as in [BIP-341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-10))

Fixed those.

💬 theuni commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2573886240)
Unfortunately it seems like the behavior around `$config_FLAGS` is just broken in depends. It uses `_INIT` values which are always appended-to.

So at the moment, the $config setting always takes precedence over what's set in depends, even if not overriding C(XX)FLAGS.

@hebasto Have you experimented with using regular cache variables as opposed to the _INIT ones?

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904615930)
The second isn't possible today, as no standard transaction output supports spending with both empty and non-empty witnesses (P2A is invalid with non-empty witness; P2WPKH/P2WSH/P2TR are invalid with empty witness).

Indeed, doesn't seem a big concern. Marking resolved.

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1904630649)
I've seen the vectors above `vFeerateHistogram`, are also simple `vector` types containing `integers`, `unsigned char`s, or `CAmount`s.

However, we do not name these vectors `vCAmounts`, `vInts`, or `vUnsignedChars`. Instead, they are named based on some context, such as `vTxFees`, `vTxSigOpsCost`, and `vchCoinbaseCommitment`.
For this reason, I believe naming this vector `vFeeFrac` does not add much clarity because thats obvious.
Instead, I consider `vFeerateHistogram` to be the closest m
...

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