💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2674241477)
Removing the milestone for now. I am happy to review pull requests or suggestions, if there are any.

💬 dergoegge commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2674257449)
Does this result in an end-to-end performance improvement? (e.g. in IBD)

💬 maflcko commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2674266272)
I don't think it is a supported use-case to leave the cmake build dir stale and append builds on it, without fully deleting it first. Not deleting the build dir is a well-known footgun unrelated to this pull, where devs ran into it already. So I think dropping the symlink commit is correct to not imply this is a supported use-case. (If someone wanted to close that footgun completely and permanantly, maybe cmake could error the build if the build directory already exists?) Unrelated from the foot
...

💬 hebasto commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2674279893)
@theuni

> Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.

I've removed the `MAIN_DEPENDENCY` options (I don't recall the motivation behind their introduction). A comparison of the resulting `build.ninja` files reveals no changes in dependencies.

💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2674288258)
In https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965186614

> The real problem here is that GetPersistentSetting also looks into the (non-mutable) configuration file. i'll add a flag to skip that and this should work as intended.

I think that's a useful flag, if only so we can distinguish the two cases.

Though I'm confused about what you intend to happen for users who have `upnp=1` in `bitcoin.conf`.

⚠️ lotican opened an issue: "Attracting investors. Protection from scam."
(https://github.com/bitcoin/bitcoin/issues/31924)
### Please describe the feature you'd like to see added.

Hello.
Please consider the possibility of creating a coin.

It will be minted at the time of purchase. And burned at the time of sale.

Which would be based on a liquidity pool in BTC and a defi contract.

When buying and selling, you can charge a small commission. And send it to the liquidity pool.

If the sold number of coins is burned at the time of sale. Then the total price of the asset will never fall.

Such a model is already worki
...

✅ willcl-ark closed an issue: "Attracting investors. Protection from scam."
(https://github.com/bitcoin/bitcoin/issues/31924)

:lock: fanquake locked an issue: "Attracting investors. Protection from scam."
(https://github.com/bitcoin/bitcoin/issues/31924)

📝 rkrux opened a pull request: "contrib: update `utxo_to_sqlite` tool documentation and comment"
(https://github.com/bitcoin/bitcoin/pull/31925)
I noticed couple discrepancies in the documentation and comments of `utxo_to_sqlite` tool while using it, this PR fixes them. More details in the commit messages.

👋 rkrux's pull request is ready for review: "contrib: update `utxo_to_sqlite` tool documentation and comment"
(https://github.com/bitcoin/bitcoin/pull/31925)

👍 theStack approved a pull request: "contrib: update `utxo_to_sqlite` tool documentation and comment"
(https://github.com/bitcoin/bitcoin/pull/31925#pullrequestreview-2632798422)
lgtm ACK e747ed989ebb7b9650ba1478d583f7f507e1083f

Thanks for fixing the outdated comments!
nit for the future: to refer code lines in the repo (second commit message body), it's better to use permalinks, as otherwise these links get stale quickly

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2674340062)
> Though I'm confused about what you intend to happen for users who have upnp=1 in bitcoin.conf.

The intent is to interpret `-upnp=1` as `-natpmp=1` and log a warning message. Pretty much like the other parameter interaction. This is what the first `if(args.IsArgSet("-upnp")) {...}` does, or is supposed to do.

🤔 marcofleon reviewed a pull request: "fuzz: add basic TxOrphanage::EraseForBlock cov"
(https://github.com/bitcoin/bitcoin/pull/31918#pullrequestreview-2632830776)
ACK 8400b742fa6dda4ad89311f547ccf50b0187e817

Ran the target and checked coverage to be sure, lgtm.

💬 maflcko commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965359990)
If the goal is to enable natpmp by default, then the `TODO: remove for version 30.0.` can be replaced by `This can be removed when the default value for natpmp is true.` ?

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2674370294)
> I don't think it is a supported use-case to leave the cmake build dir stale and append builds on it, without fully deleting it first.

What are you talking about? I am doing this constantly many times every day, when I am checking out different PRs and branches and running make and expecting binaries to be up to date when it finishes. If other developers are happy to break this assumption I'm also happy though. Just trying to point out alternatives and tradeoffs here.

💬 Sjors commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2674427722)
> I am doing this constantly many times every day, when I am checking out different PRs

Same, but I've sometimes found it unreliable, just as with autotools. For example on this branch, if you first do `-DENABLE_WALLET=OFF` and then `-DENABLE_WALLET=ON` if will fail with "Wallet functionality requested but no BDB or SQLite support available."

Baking a guix build...

No strong opinion on the symlinks.

⚠️ Prabhat1308 opened an issue: "Unable to generate coverage report using lcov on MacOs 15.3.1"
(https://github.com/bitcoin/bitcoin/issues/31927)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

While trying to get a coverage report , the report generation fails complaining of duplicate function definitions.

### Expected behaviour

Generate a coverage report for the branch.

### Steps to reproduce

```
cmake -B build -DCMAKE_BUILD_TYPE=Coverage
cmake --build build
cmake -P build/Coverage.cmake
```

### Relevant log output

```
Capturing coverage data from src
geninfo cmd: '/opt/h
...

💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2674460892)
That makes sense.

I re-tested the `bitcoin.conf` behavior by disabling the `GetPersistentSetting` bit. It prints the right log message and enables PCP. However in the GUI options dialog the check box is unchecked. And there's nothing in "options overriden by the command line". So this might be confusing, although it works.

In contrast, when I set `natpmp=1` in `bitcoin.conf` the checkbox _is_ marked.

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2674492314)
> I re-tested the bitcoin.conf behavior by disabling the GetPersistentSetting bit. It prints the right log message and enables PCP. However in the GUI options dialog the check box is unchecked. And there's nothing in "options overriden by the command line". So this might be confusing, although it works.

Yes that seems wrong, i'll see if i find a way to get the checkbox correct without spurious "write-through of bitcoin.conf settings to settings.json" behavior.

👍 theStack approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2632977865)
Light re-ACK 568fcdddaec2cc8decba5a098257f31729cc1caa