💬 theuni commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#discussion_r1944952072)
Not critically, no. I just figured it should for consistency. Is there a downside to this?

💬 theuni commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#issuecomment-2640196265)

> The `CMAKE_BUILD_TYPE` cache variable lost its help string:
>
Ugh, presumably because it's enabling c++ that sets this?

I guess that's a downside, and there may be other side-affects as well. I suppose it'd be safer to do this after enabling c++, which means letting that check be done with the wrong build type. Which realistically shouldn't ever be a problem. Agreed?

💬 hebasto commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#issuecomment-2640206091)
> > The `CMAKE_BUILD_TYPE` cache variable lost its help string:
>
> Ugh, presumably because it's enabling c++ that sets this?
>
> I guess that's a downside, and there may be other side-effects as well. I suppose it'd be safer to do this after enabling c++, which means letting that check be done with the wrong build type. Which realistically shouldn't ever be a problem. Agreed?

Given https://github.com/bitcoin/bitcoin/pull/31711#discussion_r1944952072, yes, I agree.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944976427)
Descriptor wallets never show `iswatchonly` for individual addresses. They are only `ismine` or not. That's that whole point. The watchonly distinction lives at the wallet level, not that address level.

💬 sr-gi commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640221382)
This seems to have heavily increased the run time of the rest (from ~30s to ~90s on my local setup) :/

💬 sr-gi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2640225426)
Rebased to fix merge conflicts.

Thanks for the review @tdb3 @danielabrozzoni @i-am-yuvi

💬 mzumsande commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640229392)
> Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?

I wouldn't expect that. `ConnectTip()` will call `InvalidBlockFound()` which should mark the block index as failed, and we won't revalidate / stay at the last valid block - same scenario as if it failed the script checks.

I can see the infinite loop happening only if a previously admitted block fails with `BLOCK_MUTATED` during connection, but mutation rules shouldn't be affected by a
...

💬 maflcko commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944986256)
"i.e." is latin and stands for "id est" ("that is"). I think what you want is to given an example? That'd be "e.g.", which stands for "exempli gratia" ("for example"). See also

```
src/net_processing.cpp:/** Whether this peer can only serve limited recent blocks (e.g. because
src/net_processing.cpp- * it prunes old blocks) */
```

However, I think it is fine to not list all examples here, because the list can never be known to be complete anyway. Just adding the reference to the BIP se
...

💬 instagibbs commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640247828)
@sr-gi I'll take a look

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944998630)
Thanks for the reminder, for some reason I keep thinking i.e. means "for example."

Are you suggesting s/i.e./e.g./, or removing the example? My thought was that this doc is a bit more user-facing, so more explanation or an example might be helpful.

💬 ismaelsadeeq commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2640268166)
Hmm, I am not sure whether this is a rational fee rate.

1. It's also unclear what confirmation target this "rational fee rate" is aiming for.
2. I see there is a case where you just 1000 blindly this is not rational at all, it may be that your mempool is sparse because you just started your node, and you only have 1 block worth of transactions.
3. The script you provided does not also check against the minimum relay fee rate. Hence you might provide a fee rate below that and your transaction
...

💬 ajtowns commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640271485)
> > Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?
>
> I wouldn't expect that. `ConnectTip()` will call `InvalidBlockFound()` which should mark the block index as failed, and we won't revalidate / instead will stay at the last valid block - same scenario as if it failed the script checks.
>
> I can see the infinite loop happening only if a previously admitted block fails with `BLOCK_MUTATED` during connection, but mutation rules shoul
...

💬 instagibbs commented on pull request "test: fixes p2p_ibd_txrelay wait time":
(https://github.com/bitcoin/bitcoin/pull/31759#issuecomment-2640284141)
@glozow managed to misread the changeset, nevermind

💬 naiyoma commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2640292027)
@krishpranav there's an open pr for this here -> https://github.com/bitcoin/bitcoin/pull/31252

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945028593)
Removed the example here.

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945036311)
Shortened the line length in the last push.

💬 sipa commented on issue "nSequence is not set when spending from satisfiable descriptor with relative timelock":
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2640347090)
We discussed this a bit off-line.

There are a number of issues here, and the simple ones can be solved in specialized cases, but the general problem is really a sign of a deeper issue.

The superficial problems are at least these, which stem from the specific order decisions are made: (1) coin selection, (2) transaction building, (3) input signing.
* Coin selection may be picking coins which are not (yet) spendable (assuming the intent is to spend right now, and not something to be presigned an
...

📝 instagibbs opened a pull request: "test: test_inv_block, use mocktime instead of waiting"
(https://github.com/bitcoin/bitcoin/pull/31811)
Performance issue reported in https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640221382

It seems that code as-is waits for wall-clock time to pass to synchronize mempools. Locally, sometimes the subtest takes a couple seconds, sometimes it takes an additional minute.

Just use mocktime?

⚠️ l0rinc opened an issue: "Networking tests fail on emulated big-endian systems"
(https://github.com/bitcoin/bitcoin/issues/31812)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

Running the test with an emulated big-endian system (via `docker run --platform linux/s390x -it ubuntu:24.10 /bin/bash`, as described in https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493612110), I'm seeing mismatched byte orders in IP address representations.
I've noticed this in https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2614010124, but it reproduces on mas
...

🤔 murchandamus reviewed a pull request: "Wallet: don't underestimate the fees when spending a Taproot output"
(https://github.com/bitcoin/bitcoin/pull/26573#pullrequestreview-2599346386)
Concept ACK