💬 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

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2674495672)
> re-ACK [61885ad](https://github.com/bitcoin/bitcoin/commit/61885ad9d406e25ec3b6523d01918e886996f1c3)
>
> Dropped the else: [#31622 (comment)](https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147)

@Sjors 61885ad9d406e25ec3b6523d01918e886996f1c3 commit seems to be outdated now.

💬 maflcko commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2674514881)
> What are you talking about?

The set of binaries that are built can be picked by setting configure options, so the most obvious way to end up with stale binaries is to make a build will all binaries and then another build with a subset of the binaries (intentionally or accidentally) without clearing the build dir. This is one example. Another example would be a stale cmake cache, which influences the build result (intentionally or accidentally). I am sure there are other examples.

All of
...

💬 maflcko commented on issue "Unable to generate coverage report using lcov on MacOs 15.3.1":
(https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674522975)
This is a known issue. While I don't have macOS, some reported that it was working with clang/llvm tooling.

You can try `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'` and `llvm-cov`/`llvm-profdata` and see if that works for you.

💬 vasild commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2674531786)
Almost ACK dcf9a45aab4c4a7f9f900d5dafcdfe0e9a598a38, modulo squash the two commits into one and rebase.