💬 hebasto commented on pull request "build: Fix `-Xclang -internal-isystem` option":
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1882820244)
> I guess this is meant to be an example of someone else using `-Xclang`?

Right, just an example. I forgot about #27328.

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1882825009)
I've added another commit which has pretty much the same test but using descriptor wallets i.e. a rescan through `importdescriptors`. Just like the other test, you can reproduce the bug by cherry-picking from #29019. You should get `JSONRPCEXCEPTION: Invalid or non-wallet transaction id (-5)` for the child tx.

✅ fanquake closed an issue: "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`"
(https://github.com/bitcoin/bitcoin/issues/29174)

🚀 fanquake merged a pull request: "build: Fix `-Xclang -internal-isystem` option"
(https://github.com/bitcoin/bitcoin/pull/29195)

💬 fanquake commented on pull request "build: Drop `ALLOW_HOST_PACKAGES` support in depends":
(https://github.com/bitcoin/bitcoin/pull/29203#issuecomment-1882846653)
ACK 080763a058f399583deb9dea6687e87fc1c097a9 - I can't imagine this option got any use outside our CI. It's also mostly just at odds with the idea of a self-contained dependency builder.

💬 theuni commented on pull request "build: Drop `ALLOW_HOST_PACKAGES` support in depends":
(https://github.com/bitcoin/bitcoin/pull/29203#issuecomment-1882847859)
Concept ACK. Happy to have this gone.

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1445940541)
Might be dropped now.

👍 TheCharlatan approved a pull request: "build: Drop `ALLOW_HOST_PACKAGES` support in depends"
(https://github.com/bitcoin/bitcoin/pull/29203#pullrequestreview-1810909007)
ACK 080763a058f399583deb9dea6687e87fc1c097a9

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1445941796)
thx, done

💬 theuni commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1445938519)
Unrelated to this PR, but this line is busted. gcc/clang take the last `-O` option, so `-O0` is just clobbering the `-Og` here.

🤔 theuni reviewed a pull request: "build: always set `-g -O2` in `CORE_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/29205#pullrequestreview-1810905286)
Concept ACK.

My only hesitation here was: can the user revert back to the default (no flag) behavior if desired. The gcc manpages answer this explicitly:

> Level 0 produces no debug information at all. Thus, -g0 negates -g

and

> -O0 Reduce compilation time and make debugging produce the expected results. This is the default.

So if the user uses: `./configure CXXFLAGS="-O0 -g0"`, the build should end up using:
`-g -O2 ... -O0 -g0`, which gets us back to the same behavior as if n
...

💬 fanquake commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG` & native_llvm":
(https://github.com/bitcoin/bitcoin/pull/29188#issuecomment-1882868773)
Pulled back in the commit to fix Qt for Clang < 17.

🤔 glozow reviewed a pull request: "logging: Simplify API for level based logging"
(https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1810967229)
Concept ACK based on the developer-notes. I'm a fan of having 1 not-too-verbose one that we use most of the time, and then a few others based on severity.

💬 stickies-v commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445981123)
> Ideally, LogInfo would optionally take a category.

I had that intuition at first too, but then changed my mind after reading [ajtowns's comment](https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1702318968) stating:

> My reasoning here is that the usefulness of the "category" isn't in telling us what bit of code triggered the log message (we have -logsourcelocations for that which is much more precise, and you can generally just grep for the message anyway), but in categorising
...

💬 glozow commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1445992074)
These are still [magic numbers](https://en.wikipedia.org/wiki/Magic_number_(programming)), even if they are accurate. The comment is helpful, but doesn't change the fact that this test relies on the implementation of `create_self_transfer`, making it brittle. If we were to change the implementation so that the virtual size could be something other than 104, or if we adjusted the default feerate to something other than 0.003, this test would break, even though the mempool/RPC logic hasn't changed
...

💬 Sjors commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1882943986)
Concept ACK on just adding these 150 lines of Python here, and lower the barrier for people to verify these things.

There's still some work in progress on fixing determinism, see https://github.com/fjahr/asmap-data/pull/6, but getting close!

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1446006765)
yeah I think a temporary `set<Wtxid>` within net processing is probably the easiest

💬 theuni commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1882953986)
From what I can tell, we shouldn't need this after clang-14: https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/bitcoin_runtime_lib.m4

Is that the case?

Noticed while reviewing https://github.com/hebasto/bitcoin/pull/43/commits/910fd7a64802380fe912446cd3c938d56e470f0b#diff-b937c829d4a0cf9a653f5361656ea8196b6ecef40fd033aa41a6da945e987bcdR146

@maflcko Can we just skip porting that to CMake, or am I missing something?

🤔 stickies-v reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1811002749)
Hooray for adding descriptor wallet support. Is there a reason both legacy and descriptor wallet tests aren't in the new `wallet_rescan_unconfirmed` test, though? That seems like a more logical grouping with less duplication? If there are technical objections, I think it would be good to add some extra documentation to both tests stating that the other wallet type is tested in file xxx.py, making it easier to clean up these tests e.g. when we drop legacy wallets or make other significant changes
...

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1446005072)
Is there a reason it's 110 and not `COINBASE_MATURITY`?