💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1943680609)
Actually the `-nowallet` ones needs it!! But the second one does not! Thanks for pointing this!

💬 jonatack commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1943827387)
> Let's leave it for a follow-up. It would be nice to re-think this function

Any update?

💬 theuni commented on pull request "ci: Remove no longer needed `-Wno-error=documentation`":
(https://github.com/bitcoin/bitcoin/pull/31804#issuecomment-2638304142)
It would be really nice to add comments for these in the future, whenever we add them. So it's clear when/why they can be removed.

💬 SBNovaScript commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r1943854800)
As `onion_service_target` will contain the one and only onion service target in this branch of logic, we don't need to explicitly add it to a list of onion service targets. This is especially true since `connOptions.onion_binds` is only used to make sure there is only one onion bind; we have already checked that above (line 1906).

⚠️ Randy808 opened an issue: "nSequence is not set when spending from satisfiable descriptor with relative timelock"
(https://github.com/bitcoin/bitcoin/issues/31808)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

While experimenting with miniscript expressions on regtest (on commit [a43f08c4ae](https://github.com/bitcoin/bitcoin/commits/a43f08c4ae3235f2fa460dd17a7f5f9f9842efd9/)) with the cli, I imported a descriptor with the `older` fragment:
`wsh(and_v(v:older(2),pk(tprv8ZgxMBicQKsPe5EVQ3cw3S1GdWyKxTeVvgVujejwFgJYGPucrUyPhsR2Zd4ngq3QSfB4HehpXqSPtwdoV7b8JYunU2kPai1AWUUxvb5gkrf/84h/1h/0h/1/*)))#tca
...

💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2638665261)
This also seems to be a known issue: "It has come to my attention that GCC and clang generate incompatible code
for passing an argument of an empty class type." https://itanium-cxx-abi.github.io/cxx-abi/cxx-abi-dev/archives/2015-December/002869.html

💬 ajtowns commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2638700302)
> > > * verificationprogress = just blocks/headers
> >
> > I think the problem doing this is that not all blocks "are worth the same"
>
> I think the suggestion is to keep the "tx weight" estimation, not change it. The suggestion is basically to split the "future headers estimation" from the verification progress estimation of a block in a known headers chain and return two floats.

Sure, let's pretend that's what I said :) That would be:

* t = txs in blocks we've verified
* h = he
...

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