💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314549905)
> Are you suggesting that I add a test case that checks that we dont create transaction when the tx fee as a result of the fee rate provided by settxfee is more than maxtxfee?

I’d be surprised if we don’t already have a test for that.

> Note: This wallet RPC is likely going to be removed soon.

"soon" is very relative here.

🤔 l0rinc reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3174344920)
Concept ACK

💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314454304)
not sure I understand why this was needed

💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314448739)
could we fix this in https://github.com/bitcoin-core/leveldb-subtree/pulls instead?
I have opened https://github.com/bitcoin-core/leveldb-subtree/pull/56 (I'm also fine with doing it in a different PR, just wanted to give the possibility to solve it in a cleaner way)

💬 l0rinc commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2314579166)
I didn't mean my response to be offensive before, and while still not sure I fully follow the arguments, I agree that stabilizing the formatting and unifying class/struct braces are separate concerns, so I have removed the unification, only kept the change making formatters explicit.

💬 l0rinc commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#issuecomment-3243270669)
Kept the formatter config only without the class/struct brace unification, it's ready for re-review, thanks for the comments.

💬 hodlinator commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3243290826)
Tested but did not dig deep into the code for now. Spent more than a day adding debug code in Bitcoin Core and fighting with my OpenWrt router (https://github.com/openwrt/packages/issues/17871#issuecomment-3243222244). Now that the router has been fully reset without any lingering configs from prior major versions, PCP & NATPMP is actually working.

On master (in 7/7 attempts roughly):
```
₿ cmake --build build -j16 --target=bitcoind && ./build/bin/bitcoind -regtest -debug=net
...
Bitcoin
...

⚠️ l0rinc opened an issue: "InitError() doesn't always halt node startup when blockchain state exists"
(https://github.com/bitcoin/bitcoin/issues/33276)
As described in https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305592114, when `InitError()` is triggered during initialization, the node prints the error but sometimes continues running, if blockchain data already exists in the datadir.

```bash
rm -rfd demo build && mkdir -p demo && cmake -B build && cmake --build build -j$(nproc)
# empty state fails with error
build/bin/bitcoind -datadir=demo -stopatheight=100 -i2psam=invalid_addr
# init state with 10 blocks
build/bin/bitcoind -da
...

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2314600353)
Opened https://github.com/bitcoin/bitcoin/issues/33276, please resolve this comment

💬 hodlinator commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3243304152)
> I don't see any pmp fallbacks happening in logs?

We only fall back to NATPMP for pcp_err == `UNSUPP_VERSION`:

https://github.com/bitcoin/bitcoin/blob/b2d07f872c58af9cfdf9f9a4af0645376f9b98cb/src/mapport.cpp#L77-L81

💬 achow101 commented on pull request "kernel: pre-29.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/31978#issuecomment-3243310067)
> Note that `du -h` produces 1024-based units, while `m_assumed_blockchain_size` and `m_assumed_chain_state_size` expect multiples of 10^9 bytes, which means adding 4.8% to `M` results, and 7.4% to `G` results.

No? We use multiples of 1024.

💬 achow101 commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314606198)
Seems low, would expect 244.

```
$ du -csh --apparent-size testnet3/chainstate/ testnet3/blocks/
16G testnet3/chainstate/
205G testnet3/blocks/
221G total
```

💬 achow101 commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314607768)
Seems low, would expect 20.

```
du -csh --apparent-size signet/chainstate/ signet/blocks/
2.8G signet/chainstate/
15G signet/blocks/
18G total
```

💬 achow101 commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314607076)
I think testnet4 blockchain size is bigger

```
du -csh --apparent-size testnet4/chainstate/ testnet4/blocks/
928M testnet4/chainstate/
20G testnet4/blocks/
20G total
```

💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3243349872)
> Hmm, I forgot that `CachedTxIsFromMe` does not use `IsFromMe`. Might need to change that in this PR as well.

I've pulled in 2 commits from #27865 which drop `CachedTxIsFromMe` and replace it with an `IsFromMe` check which is stored on disk.

> In both places we effectively treat it as `is_mine = IsMine(outputs) || IsFromMe(inputs)`.

While that's true, moving `IsFromMe` into `IsMine` does change `IsMine`'s semantics in other places, so I'm not sure that that would necessarily be correct
...

💬 achow101 commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243356083)
The `id` attribute is required by the XLIFF 1.2 spec: https://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#trans-unit

💬 achow101 commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3243360930)
> Was "Translation Memory Fillup" enabled then?

Only if it is by default. I don't know where that setting is.

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314667768)
Done

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668014)
Hm, I am not sure on this one. It sounds like this is so unlikely that the overflow value would probably go without an implementation of how to use it until something that triggers this actually happens. E.g. even if something goes on that looks suspicious the next release may be a few months out so the attack could already be carried out. In that case users would need to resync again anyway if I’m not mistaken. So I would prefer to only add this if there is a clear path to implement the overflo
...

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668306)
Right, this code is "belts-and-suspenders" since we should never end up here since the base index should rewind us in this case before entering `CustomAppend`. So I will turn this into an error and drop the hashkey query below since we aren't recovering from that anyway.

This isn't something that is added here so it could be a separate PR but I will add it here since it simplifies the code a bit.