💬 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

💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1483728729)
@ajtowns
> > this PR proposes deleting an option that .. (2) does no harm to the node operator.
>
> This isn't true. Running a non-default mempool makes it easier to put different txs in your mempool compared to the majority of the network, which:
>
> * makes it easier to fingerprint your node and its network peers

How specifically does turning *on* full-rbf enable that attack?

> * creates pinning vectors

Can you explain what exactly you mean by this in the context of fu
...

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1148307887)
@brunoerg, yes, that should still bring some improvement. But only push it into the vector if we have visited all addresses in that bucket and found 0 from the requested network because it may happen that we visit a bucket with an address from the requested network, but only pick it up on some of the subsequent iterations, not from the first one. Change it to `std::unordered_set` so that lookup is fast (`O(1)`).

💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1483735264)
@ariard
> * I think the conciliation of the 0confs use-case with new policy regime like nversion=3 has not been very discussed (e.g I believe 0conf software might have to upgrade themselves to reject future nversion=v3 transactions).

To be clear, 0conf users have to "upgrade" every single time mempool policies/conditions change for any non-trivial amount of hashing power. Pretty much any mempool policy change can be exploited to double-spend unconfirmed transactions. That's why the only ent
...

💬 kyranjamie commented on pull request "BIP-322 basic support":
(https://github.com/bitcoin/bitcoin/pull/24058#discussion_r1148338733)
I get this value in my implementation, yet it is not reflected in the BIP

💬 hebasto commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483792947)
> 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'

Mind providing steps to reproduce it, including your build environment details?

💬 fanquake commented on pull request "guix: combine and document `enable_werror`":
(https://github.com/bitcoin/bitcoin/pull/27326#issuecomment-1483802220)
> as it was when you introduced those "multiple funcs", they will be useful again.

It was needed when we were building 2 different glibcs, in two different ways. Now we're building a single glibc, in one way, hence consolidating. We can speculate about what might happen in future, but I can't see a reason (looking at current releases), to return to multiple builds. The only reason it was done this way initially, was for RISC-V support. I also don't know why we'd turn -Werror back on again, it's
...

💬 Ayms commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483821598)
@1440000bytes

> dont get mad else some people from this repo might write things about you. Getting mad is only reserved for a few people here

What do you mean and what is your problem? Write things about me?

💬 1440000bytes commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1483822456)
@Ayms Edited. Sorry it wasn't for you.

💬 vincenzopalazzo commented on issue "core stops to run with `Failed to read block` error":
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1483839533)
yeah I will try asap

💬 hebasto commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1483841839)
> I'm seeing an additional unrelated problem with linking with system clang, but I'll PR the solution to that separately as it's not as straightforward as this.

FWIW, with the following diff:
```diff
--- a/depends/hosts/darwin.mk
+++ b/depends/hosts/darwin.mk
@@ -38,7 +38,9 @@ clangxx_prog=$(shell $(SHELL) $(.SHELLFLAGS) "command -v clang++")
llvm_config_prog=$(shell $(SHELL) $(.SHELLFLAGS) "command -v llvm-config")

clang_resource_dir=$(shell clang -print-resource-dir)
+llvm_bin_d
...

📝 martinus opened a pull request: "refactor: extract CCheckQueue's data handling into a separate container "Bag""
(https://github.com/bitcoin/bitcoin/pull/27331)
`CCheckQueue` has stored its work items in a `queue`, but made no guarantee about the order of elements in that container. This PR extracts that data storage handling into a separate container class `Bag`. This is pure refactoring, the result should have a better separation of concerns, adds tests for the new container, and makes it now easier to separately test and optimize the container.

👍 hebasto approved a pull request: "guix: combine and document `enable_werror`"
(https://github.com/bitcoin/bitcoin/pull/27326)
ACK 4becee396f3bda40832138dd1aaa90368ed31857, the diff is correct.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1483882249)
Regarding CI failures,` ./test/functional/wallet_create_tx.py` passes locally, actioned manually re-run and I'm checking the fuzzer one `cirrus-ci-build/ci/scratch/qa-assets/fuzz_seed_corpus/http_request" failed with exit code 77`.

💬 Ayush170-Future commented on issue "ci: Cleanup ci/test/01_base_install.sh":
(https://github.com/bitcoin/bitcoin/issues/27321#issuecomment-1483895345)
Hello @MarcoFalke!
I'm new to Bitcoin Core development as well as Bash scripting. But, based on the requirements and the script, I believe I can tackle this.

I only have a few questions:
1) I discovered other files in the test directory, such as ci/test/04_install, also contain the CI_EXEC_ROOT and CI_EXEC alias. So, are they also to be modified?

2) After reading your comment in the script, I believe I can just remove the CI_EXEC_ROOT and CI_EXEC parts of different commands in the script
...

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1148418379)
nit: unused variable, can be removed
```suggestion
```

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1148418808)
pedantic-nit:
```suggestion
// Note that each transaction's ancestor size is 1 or 3, and each descendant size is 1, 2 or 3.
```
The descendant count of 2 is the rare case here, only applying for parent-txs txp0 and txp49
(for the fun of it, created a functional test for verifying this: https://github.com/theStack/bitcoin/blob/functional_test_mempool_zigzag_playground/test/functional/mempool_zigzag.py).

👍 TheCharlatan approved a pull request: "guix: combine and document `enable_werror`"
(https://github.com/bitcoin/bitcoin/pull/27326)
ACK 4becee396f3bda40832138dd1aaa90368ed31857

👍 Ayaan7211 approved a pull request: "MiniTapscript: port Miniscript to Tapscript"
(https://github.com/bitcoin/bitcoin/pull/27255)