💬 laanwj commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2139605484)
Concept and code review (but untested, will use this to make a testing harness for #30043) ACK 77e34ded543e19ba27cab8d0cfc464af27f04bbf

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620771700)
That depends on `T`'s move constructor. Typically, this won't do anything, but it's not impossible for a `T::T(const T&&)` constructor to exist (and this may make sense if `T` has `mutable` member variables).

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1620771908)
Is it though? If the user has not specified that he wants no automatic connections (`-maxconnections != 0`) I'd argue he'd expect them to happen.

In any case, I think this is such an edge case that it is not worth blocking the PR on it. If you have a strong opinion on this, I'm happy to drop it and squash the rest of the commit to one of the previous ones

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620773911)
Actually, no, you're right. Even if this does anything, it won't be the desired behavior. Will remove.

💬 maflcko commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#discussion_r1620825787)
nit: Could make sense which version is "recommended" or "tested". I presume the version used in CI? If so, could refer to that.

🚀 fanquake merged a pull request: "clang-tidy: Add `bugprone-move-forwarding-reference` check"
(https://github.com/bitcoin/bitcoin/pull/30199)

📝 BrandonOdiwuor opened a pull request: "Enhance signet chain configuration in bitcoin.conf"
(https://github.com/bitcoin/bitcoin/pull/30203)
## !! Early draft for feedback on approach !!

Follow up to https://github.com/bitcoin/bitcoin/pull/29838 following https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2063151580 and https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2070170967

This PR builds upon the changes introduced in https://github.com/bitcoin/bitcoin/pull/29838, which enabled the ability to run multiple Signet instances with distinct data directories. Now, it extends this functionality by allowing user
...

💬 ismaelsadeeq commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1620854919)
```suggestion
send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))
# Ensure UTXO is enough to account for the required fee
assert_greater_than_or_equal(send_value, 0)
```

💬 ismaelsadeeq commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1620853166)
Why are you incrementing by 3 twice
Please use `WITNESS_SCALE_FACTOR` const instead of just 4.

Magic numbers makes my head hurts 😃

Should be worth it to define 3 in wallet as `PADDING_OFFSET` ?

🤔 ismaelsadeeq reviewed a pull request: "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)"
(https://github.com/bitcoin/bitcoin/pull/30162#pullrequestreview-2088475662)
Concept ACK

This change is really useful and needed.

I came across this issue while working on another PR and had to fix it in my PR #30157. You can see the relevant commit https://github.com/bitcoin/bitcoin/pull/30157/commits/93146927acf8ce7d929598b15224ef91ece6768c.

Third commit 93527b82e70c8e19d7317ce5287b0ee2a0020f1b looks good to me.

I will do a thorough review of the first two commits.

💬 instagibbs commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1620865246)
few remaining spots that I can see:
```
src/test/fuzz/package_eval.cpp:176: tx_mut.nVersion = fuzzed_data_provider.ConsumeBool() ? 3 : CTransaction::CURRENT_VERSION;
src/test/fuzz/tx_pool.cpp:229: tx_mut.nVersion = fuzzed_data_provider.ConsumeBool() ? 3 : CTransaction::CURRENT_VERSION;
src/test/txvalidation_tests.cpp:289: mtx_many_sigops.nVersion = 3;
src/test/util/txmempool.cpp:121: if (tx_info.tx->nVersion == 3) {
src/test/util/txmempool.cpp:136:
...

🤔 furszy reviewed a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2088523996)
Code review ACK eeea0818c1

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1620894922)
Ended up taking this, since the limit will be kept in the end.

Thanks!

💬 stickies-v commented on pull request "Fix typos in 36 files | Almost only documentation":
(https://github.com/bitcoin/bitcoin/pull/30188#issuecomment-2139858597)
> I think this should be closed.

Agreed. Concept NACK.

👋 Sjors's pull request is ready for review: "Introduce Miner interface"
(https://github.com/bitcoin/bitcoin/pull/30200)

💬 Sjors commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2139890557)
This seems very useful. I'll try to use it for the (currently very brittle) `Sv2Transport` tests in #29432, and review it along the way.

📝 fanquake opened a pull request: "depends: consolidate dependency docs"
(https://github.com/bitcoin/bitcoin/pull/30204)
Adds missing `g++` for macOS. This is needed by Qt:
```bash
Configuring qt...
Creating qmake...
gmake[1]: Entering directory '/bitcoin/depends/work/build/arm64-apple-darwin/qt/5.15.14-4bca24c8f89/qtbase/qmake'
gmake[1]: g++: No such file or directory
gmake[1]: *** [Makefile:250: main.o] Error 127
```

Removes `bsdmainutils` for macOS, as couldn't see where it's needed, and could complete a build with it uninstalled.
Can re-add if someone knows what it's needed for.

`xz-utils` was al
...

💬 maflcko commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#issuecomment-2139941683)
utACK e99ecf01a54dd0d1b3b384b70125caa819724578

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620955671)
It's a shame that this triggers a reallocation when `m_offset == 0`. Might it be worth optimizing for that case?

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1620962324)
I tried to track down if/how it could be a race condition, but given that it's in init and all locks seems to be in place, I don't see how that could possibly be the case.