💬 kevkevinpal commented on pull request "miniscript: convert non-critical asserts to CHECK_NONFATAL":
(https://github.com/bitcoin/bitcoin/pull/31727#issuecomment-2638757606)
Concept ACK

lightly reviewed the code and looks good to me

💬 tofutim commented on issue "guix: failure on Kubuntu 24-10: error: mount: mount "none" on "/tmp/guix-directory.VEMlin": Permission denied":
(https://github.com/bitcoin/bitcoin/issues/31202#issuecomment-2638760471)
Ubuntu 24.04.1 LTS here, at first attempt
```
guix shell: error: mount: mount "none" on "/tmp/guix-directory.WvZoWc": Permission denied
```
w/ AppArmor stopped and disabled, and reboot
```
...bind-mounted in container to: '/outdir-base/x86_64-linux-gnu'
ADDITIONAL FLAGS (if set)
ADDITIONAL_GUIX_COMMON_FLAGS:
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS:
ADDITIONAL_GUIX_TIMEMACHINE_FLAGS:
guix shell: error: clone: 2114060305: Permission denied
```

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2638798337)
Changed:
* Added `GetAncestorsUnion` and `GetDescendantsUnion`, which act like their non-union version, but take in a span of multiple `Ref*`, and return the `Ref*` for the union of their ancestors/descendants

(requested by @sdaftuar while rebasing #28676)

🤔 glozow reviewed a pull request: "test: make sure we are on sync with a peer before checking if they have sent a message"
(https://github.com/bitcoin/bitcoin/pull/31760#pullrequestreview-2597653967)
ACK 3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08

🚀 glozow merged a pull request: "test: make sure we are on sync with a peer before checking if they have sent a message"
(https://github.com/bitcoin/bitcoin/pull/31760)

💬 ajtowns commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2638834293)
> > It might be good to replace this comment [...] with the actual post-this-PR upgrade-behaviour, rather than just deleting it?
>
> I couldn't come up with any useful comment to replace it with. The original comment is a warning about the surprising original behavior. Now that the surprising behavior is removed, it makes sense to just delete the warning and not replace it with "this does what you'd expect"?

I guess it's not obvious to me what you should expect here.

I believe the curre
...

🤔 glozow reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2597709421)
utACK 386eecff5f14d508688e6e7374b67cb54aaa7249, nonblocking nits. I do think the release notes should be clarified more

💬 glozow commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1944136249)
Maybe also clearly state how they interact? What if you set `-blockreservedweight` higher than `-blockmaxweight`?

💬 glozow commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1944134062)
nit: maybe all of these should live in miner.h?

🤔 glozow reviewed a pull request: "test: fixes p2p_ibd_txrelay wait time"
(https://github.com/bitcoin/bitcoin/pull/31759#pullrequestreview-2597720235)
ACK 1973a9e4f1dfba57051135d6e1bca80979de3879

> meta: do we ever want a test to bumpmocktime without setting it first?

No, they always need to setmocktime first. As documented: https://github.com/bitcoin/bitcoin/blob/ae9eaa063b68660cbfef336de52e23a3c2dfda92/test/functional/test_framework/test_node.py#L832-L834

🚀 glozow merged a pull request: "test: fixes p2p_ibd_txrelay wait time"
(https://github.com/bitcoin/bitcoin/pull/31759)

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1944167262)
I go to the `\'` way instead because python linters often yell w/ double quotes but I'm fine w/ double quote if it's fit the CI

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1944226046)
I guess it's a philosophical question. There's mempool policy and there's the defaults we set for miners, which typically match. But the `*_WEIGHT` constants here have no effect on the mempool, so are they policy?

But there's another consideration: `miner.{h,cpp}` consists mostly of utility code, and so doesn't seem like a good place to put these important constants. We could introduce `mining_defaults.h`, but it seems simpler to keep them here.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1944231008)
That's an absurd thing to do, so I'm worried that explaining it just makes the text hard to follow. That could miners to ignore the import warning about setting this value too low.

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944235910)
Hmm the getaddressinfo for this address displays it as false.

```
{'address': 'bcrt1q4dsf6kwgrraj6v6zvcr4a4cwlrfdvmnd0mh8yqcgpzrnryep5l5s4fr46d', 'scriptPubKey': '0020ab609d59c818fb2d334266075ed70ef8d2d66e6d7eee7203080887319321a7e9', 'ismine': True, 'solvable': True, 'desc': 'wsh(multi(1,[978e2eb7]0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#str4m9tu', 'parent_desc': 'wsh(multi(1,0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#qqala45t', 'iswatcho
...

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944239980)
I'm not sure how `iswatchonly` is supposed to behave in watch-only _descriptor_ wallets. cc @achow101

💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2639084458)
Thinking more about the motivation, I agree with it, but I fail to see the connection to removing this RPC:

* The RPC doesn't rule out dynamic fee estimation. In fact, I expect some users not using Bitcoin Core's fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).
* If a user wants to shoot themselves into the foot by setting the feerate to a constant
...

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1944282152)
done

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1944282491)
done

💬 maflcko commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639134040)
I wonder if there is a way to detect those at compile or link time. Other than that, printing a warning when mixing two compilers when compiling with depends may be useful?