💬 1440000bytes commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483310633)
> Well, reading this 10 times, I am not sur to see what it has to do with #27043

I assume this was a test run for real pull pull request which will be controversial for sure, dont get mad else some people from this repo might write things about you. Getting mad is only reserved for a few people here.

💬 hebasto commented on pull request "guix: combine and document `enable_werror`":
(https://github.com/bitcoin/bitcoin/pull/27326#issuecomment-1483359577)
> I don't see a reason to keep them separated, need multiple funcs etc.

In the future, if we will have multiple glibc versions again, as it was when you [introduced](https://github.com/bitcoin/bitcoin/pull/25076/commits/c9c5b3060d2edb47ebfa7974fdde3154036717c2) those "multiple funcs", they will be useful again.

I mean, while this change improves code readability, it hurts maintainability a bit, doesn't it?

📝 theuni opened a pull request: "depends: fix osx build with clang 16"
(https://github.com/bitcoin/bitcoin/pull/27328)
Current build (using forced system clang as a test) results in:

> error: unknown argument: '-internal-externc-isystem/opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/include'

For some reason the previous syntax worked with clang 15 and below, but clang 16 requires that the option and value are properly separated.

See [here for an example of upstream using this syntax](https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/crash-report-with-asserts.c#L9).

Th
...

💬 hebasto commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483390448)
#27314?

💬 hebasto commented on pull request "build, qt: Fix handling of `CXX=clang++` when building `qt` package":
(https://github.com/bitcoin/bitcoin/pull/27314#issuecomment-1483392086)
The PR descriptions has been updated with Guix build hashes.

👍 TheCharlatan approved a pull request: "depends: qrencode 4.1.1"
(https://github.com/bitcoin/bitcoin/pull/27312)
Code review ACK eb1c3adf38cb71c3e6f298a61871738c4919b4a1

I checked that the additional cflags indeed suppress errors when building with `clang-15`. Also went though the changed configure flags and did a quick sanity check in the gui.

📝 mxaddict opened a pull request: "Added clangd .cache and compile_commands.json to .gitignore"
(https://github.com/bitcoin/bitcoin/pull/27329)
Added cause these were always getting created whenever I was using clangd in the project

💬 willcl-ark commented on pull request "Added clangd .cache and compile_commands.json to .gitignore":
(https://github.com/bitcoin/bitcoin/pull/27329#issuecomment-1483411858)
Concept NACK.

See https://github.com/bitcoin/bitcoin/pull/27275 for how this should be done.

💬 mzumsande commented on issue "test_bitcoin: ./chain.h:261: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed.":
(https://github.com/bitcoin/bitcoin/issues/27320#issuecomment-1483430905)
maybe not a cosmic ray after all (#26613)

💬 mxaddict commented on pull request "Added clangd .cache and compile_commands.json to .gitignore":
(https://github.com/bitcoin/bitcoin/pull/27329#issuecomment-1483467694)
>

Makes sense

✅ mxaddict closed a pull request: "Added clangd .cache and compile_commands.json to .gitignore"
(https://github.com/bitcoin/bitcoin/pull/27329)

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136045597)
nit, it would be more logical for this check to be just after `to_process_count` is incremented below.

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136047228)
nit, I think you could eliminate the `to_process_count` variable, and just use the vector size instead.
```suggestion
for (size_t i{0}; i < clustered_txs.size(); ++i) {
```

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136051666)
That's fine, suggestion withdrawn :)

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136048292)
nit, I think you can remove this code. (It's nice to eliminate special-case code, not only for simplicity, but to reduce testing.)

💬 theuni commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483641234)
> #27314?

I'm not sure I see the connection?

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1483714301)
Updated changes:

- Moving the uri parsing validation from the http_request_cb to the HTTPRequest constructor, while doing this realised that it's better to validate the result of `evhttp_request_get_uri()` also at that time.

@vasild, @stickies-v: please let me know what you think and I'll update title & description of the PR and will separate the code in 2 commits (proper fix & non-functional optimisation).

💬 petertodd commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483723052)
@ajtowns
> NACK. If someone wants to allow a bare OP_RETURN and nothing more they can set `-datacarriersize=1`. There's no reason to ignore someone setting `-nodatacarrier` to indicate they don't want OP_RETURN outputs in their mempool.

Like I said, bare `OP_RETURN` outputs are *not* data carrying outputs. Treating them as such is misleading.

Second, if you are going to take that attitude, why do we even have the `-nodatacarrier` option? Setting `-datacarriersize=0` will also prevent ba
...

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1148304529)
@mzumsande, I think you are missing my point, let me try to rephrase it: now on `master`, from `ThreadOpenConnections()` we call `Select()`. With the parent of this PR, we will be calling `Select(network)` which will bring some security (good) + some slowdown (bad).

Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.

What
...

👍 vasild approved a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214)
ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6

Maybe other reviewers would be interested in the performance discussion:
https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1130844842