💬 furszy 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_r1904521854)
+1. Something like this would be helpful. I've received a few messages sharing these logs without knowing what to do next.

👍 ryanofsky approved a pull request: "test: Add test for rpcwhitelistdefault"
(https://github.com/bitcoin/bitcoin/pull/29858#pullrequestreview-2532726241)
Code review ACK fcf0ead6cd358fd35909e1102bdae2c176b0ace6. I left suggestions below, but none are important and they can be ignored. I think it is better to have this test coverage for this than not to have it, and no need to spend too much time iterating on the PR just to improve test code quality which is already decent.

Note that this PR does not provide any coverage for the case where rpcwhitelistdefault setting is unset, only for the cases where it is explicitly set to 0 and explicitly se
...

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904534581)
In commit "test: Add test for rpcwhitelistdefault" (fcf0ead6cd358fd35909e1102bdae2c176b0ace6)

Since this code is changing configuration that affects all future tests I think it would be less confusing if it were moved out of this individual test method and into the main `run_test` method. Otherwise it is not clear that calling this particular test method has side effects for calls to future test methods.

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904497634)
In commit "test: Add test for rpcwhitelistdefault" (fcf0ead6cd358fd35909e1102bdae2c176b0ace6)

I think it would be good to change this line to if `user[2] is not None:` and change the new user's whitelist above from `""` to `None`. This should be clearer and less confusing because `""` looks like an empty whitelist, not an unspecified whitelist, and "" is already used elsewhere in the test to check empty whitelists. `None` would be clearer than `""` as a way to represent unspecified whitelists
...

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904493624)
In commit "test: Add test for rpcwhitelistdefault" (fcf0ead6cd358fd35909e1102bdae2c176b0ace6)

This function mostly contains moved code, but is confusing because it is referencing a hardcoded index in lists that are not defined here. Would suggest replacing this with a shorter version:

```python
def get_permissions(whitelist):
return [perm for perm in whitelist.replace(" ", "").split(",") if perm]
```

And replacing `get_permissions(user)` with `get_permissions(user[2])` at callsit
...

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904542876)
In commit "test: Add test for rpcwhitelistdefault" (fcf0ead6cd358fd35909e1102bdae2c176b0ace6)

Would probably make test clearer if added this new user to `self.strange_users` instead of `self.users` so it would not be necessary for the later tests in this file to need special cases like changing `for user in self.users` to `for user in self.users[:2]` and needing to hardcode `self.users[2]`.

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904514340)
In commit "test: Add test for rpcwhitelistdefault" (fcf0ead6cd358fd35909e1102bdae2c176b0ace6)

I don't understand the meaning of the COMMON_PERMISSIONS constant, and it seems it might not have have any clear meaning. I think it would probably clearer to drop the constant and write the list literally in the two places where it's used. Since other code in the tests is already referencing method names directly it would seem clearer if they did that consistently.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1904565515)
Ah, fixed.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1904565593)
Removed

👍 TheCharlatan approved a pull request: "rpc: fix mintime field testnet4, expand timewarp attack test"
(https://github.com/bitcoin/bitcoin/pull/31600#pullrequestreview-2532864851)
ACK 5ba770089e9ecf82645ae60aa2bc7f26085a5573

💬 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#discussion_r1904579870)
Done

💬 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#discussion_r1904579937)
Done

💬 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#discussion_r1904580002)
Done

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