💬 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.

💬 theuni commented on pull request "depends: qt 5.15.14 and fix macOS build with Clang 18":
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2139978109)
Is the bump to .14 also for clang 18, or is that just incidental?

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620982315)
Actually, same comment for `operator=` as well. Maybe it'd be worth a special case in `Reallocate()` itself instead.

💬 fanquake commented on pull request "depends: qt 5.15.14 and fix macOS build with Clang 18":
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2139993780)
> or is that just incidental?

Mostly incidental. We've got to rebuild all Qts after dropping the android patches changes from #30049, so seemed like a convenient time. I also try to bump to the newest source (if possible) before introducing new patches.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1620986696)
Are you imagining something like `realloc` on the buffer? AFAIK that just doesn't exist in C++; if you want to shrink an object, you need to construct a new object of smaller size and copy over.

📝 vasild opened a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205)
Put the generic parts from `StaticContentsSock` into a separate class `ZeroSock` so that they can be reused in other mocked `Sock` implementations.

Add a new `DynSock` whose `Recv()` and `Send()` methods can be controlled after the object is created. To achieve that, the caller/creator of `DynSock` provides to its constructor two pipes (FIFOs) - recv-pipe and send-pipe. Whatever data is written to recv-pipe is later received by `DynSock::Recv()` method and whatever data is written to the sock
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620997477)
> Making a separate PR could help

Done, the first two commits from https://github.com/bitcoin/bitcoin/pull/26812 extracted into a separate PR: https://github.com/bitcoin/bitcoin/pull/30205

💬 theuni commented on pull request "depends: consolidate dependency docs":
(https://github.com/bitcoin/bitcoin/pull/30204#issuecomment-2140037452)
> 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.

IIRC this was needed for hexdump, which is needed to build some tests. Maybe it's already installed by something else?