💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2061096369)
In commit "refactor: Operate on bytes instead of bits in Asmap code"

Simpler:

```c++
asmap_vec.push_back(std::byte(buffer[1 + i]));
```

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2061101276)
In commit "refactor: Operate on bytes instead of bits in Asmap code"

This doesn't make sense anymore. You can't slice off individual bits from an `std::byte`.

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2061109899)
"In commit "refactor: Operate on bytes instead of bits in Asmap code":

I think it would be helpful to have a bit getter helper function:

```c++
inline bool GetBit(std::span<const std::byte> bytes, uint32_t bitpos) noexcept
{
return (std::to_integer<uint8_t>(bytes[bitpos / 8]) >> (bitpos % 8)) & 1;
}
```

And then for example:
```c++
...
bit = GetBit(data, bitpos++);
...
```

and

```c++
...
if (GetBit(ip, ip.size() * 8 - bits)) {
--bits;

...

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2061094811)
In commit "refactor: Operate on bytes instead of bits in Asmap code"

Simpler:
```diff
--- a/src/netgroup.cpp
+++ b/src/netgroup.cpp
@@ -90,18 +90,18 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const
if (address.HasLinkedIPv4()) {
// For lookup, treat as if it was just an IPv4 address (IPV4_IN_IPV6_PREFIX + IPv4 bits)
for (int8_t byte_i = 0; byte_i < 12; ++byte_i) {
- ip_bits[byte_i] = static_cast<std::byte>(IPV4_IN_IPV6_PREFIX[by
...

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2061114715)
I don't think this is needed. `TxGraph` doesn't promise a specific ordering of equal-feerate diagram segments. The implementation is allowed to be more specific than the interface promises.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2061114932)
No, because it's computing the diagram for the linearization `vec1`, which is constructed using the real graph's transaction comparison function.

It's just using `sims[0].graph` to pull the feerate information from.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2061115163)
Sure, the caller is responsible for not violating the contract of the interface.

🚀 hebasto merged a pull request: "Replace stray tfm::format to cerr with qWarning"
(https://github.com/bitcoin-core/gui/pull/868)

💬 hebasto commented on pull request "build: Fix `macdeployqtplus` after switching to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/32287#discussion_r2061202483)
Thanks! Adjusted as well.

📝 luke-jr opened a pull request: "Bugfix: Miner: Don't reuse block_reserved_weight for "block is full enough to give up" weight delta"
(https://github.com/bitcoin/bitcoin/pull/32355)
PR #30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block:

```diff
if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
- m_options.nBlockMaxWeight - 4000) {
+ m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
// Give up if we're close to full and haven't succeeded in a while

...

💬 shahsb commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#discussion_r2061229822)
I do not see the call to the function `DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor()` being made in this test case?

💬 shahsb commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2831976141)
Concept ACK

💬 hebasto commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2831979533)
> [@hebasto](https://github.com/hebasto) can you followup with all the questions asked in this thread:
>
> [#31491 (comment)](https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2573886240)
>
> It'd be good to get some clarification on which behaviour is meant to work.

@theuni

> [@hebasto](https://github.com/hebasto) Have you experimented with using regular cache variables as opposed to the _INIT ones?

Cache variables are designed to be set by the user. With the suggested approach
...

💬 polespinasa commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2061240293)
I like the new approach!

What do you think about saving the hash and not the heigh? In case of a reorg we could be at the same height and think that nothing happened.

```
if (chache_estimate && activeTip->phashBlock == last_tip)
...

📝 Bue-von-hon converted_to_draft a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936)
Added support for creating v3 raw transaction:
- Overloaded to include additional parameter

this resolves #31348

🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2795872932)
Code review a7f76b28a1293929d05f15e0a9276d4ec3687c92.
I reviewed focussed on new commits that are added and will do a subsequent review that finalises my understanding of the whole diff.

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2061247627)
I feel this assert should come right after line 421 because that's where `sighash` can be set last, after that its value is only read.

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2061250526)
These `if` blocks here in this `else` can possibly check for `SIGHASH_ALL` for taproot inputs, which seems ok to me as I gather from the functional test and I have not found anything yet in the [BIP 341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki) that contradicts it.
Also, the `CheckSchnorrSignature` function disallows only `SIGHASH_DEFAULT` to be explicitly mentioned, so `SIGHASH_ALL` is fine here I believe.

However, I believe it'd be nice to check the size of the sign
...

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2061258027)
Nit (& for 2a721dcfae44cb0fef6825e773e5895cb87a91a5 commit message): Because it's easier to get the context from the log message instead of having to find in the test.

```diff
- self.log.info("Test sighash type mismatches")
+ self.log.info("Test sighash type mismatches in process psbt rpcs")
```

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2061262878)
Note for this commit: 14e35e675c576c770641160e982713dda4032bd8

I _think_ this is added because later in the commit 94fb539f4e22ed4ec9a65f90db26cb81e5b8749a, the last element of the partial sig is used to check for the sighash type match while signing the PSBT input for which this signature validity guarantee is needed.

It'd be nice to mention something around this in the commit message to aid review (& for future reference).