💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1852711192)
Re https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1852688371

> I'm actually not sure what is this referring to. Running nm ./src/util/libbitcoin_util_a-sock.o --undefined-only I see sock.cpp is using LogInstance, NodeClock, and SysErrorString which should all be part of the util library.

Thanks for actually verifying, I just assumed that the `common/system.h`include was actually used :(.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1852712791)
I guess I didn't catch them all locally. Pushed a change to the last (linter commit). Also fixed the broken noise handshake test. Replaced `usleep` with `UninterruptibleSleep` to keep Windows happy.

@hebasto any idea how to handle `D:\a\bitcoin\bitcoin\src\common\sv2_noise.h(92,13): error C3646: 'WriteMsg': unknown override specifier `?

```cpp
class Sv2CipherState
{
public:
Sv2CipherState() = default;
...
ssize_t WriteMsg(Span<std::byte> msg);
```

💬 maflcko commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852715761)
Should be fine to squash a 2-line diff into one commit?

💬 hebasto commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852717697)
> Should be fine to squash a 2-line diff into one commit?

Sure :)

💬 achow101 commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1852720898)
ACK 37c75c58202f89b752500f76c872d7f8caf6bdb3

👍 TheCharlatan approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1778364556)
Re-ACK 6db04be102807ee0120981a9b8de62a55439dabb

💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1852741252)


On December 12, 2023 5:49:19 PM GMT+01:00, Unhosted Marcellus ***@***.***> wrote:
>> > The original Satoshi client didn't allow embedding data onchain, this has always been an exploit.
>>
>> Bitcoin was released with no standardness rules at all. You absolutely could embed data onchain. You could even publish arbitrary data to the UTXO set easily.
>
>That was poor wording, but was precisely my point.
>
>Satoshi introduced OP_RETURN as a mechanism to prove that some sats were provably unspenda
...

📝 maflcko opened a pull request: "test: Actually fail when a python unit test fails"
(https://github.com/bitcoin/bitcoin/pull/29068)
Currently python unit test failures are ignored.

Fix this.

💬 maflcko commented on pull request "test: Actually fail when a python unit test fails":
(https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852744258)
Can be tested with something like:

```diff
diff --git a/test/functional/test_framework/crypto/bip324_cipher.py b/test/functional/test_framework/crypto/bip324_cipher.py
index 56190647f2..56211e231d 100644
--- a/test/functional/test_framework/crypto/bip324_cipher.py
+++ b/test/functional/test_framework/crypto/bip324_cipher.py
@@ -89,7 +89,7 @@ class FSChaCha20Poly1305:
# Test vectors from RFC8439 consisting of plaintext, aad, 32 byte key, 12 byte nonce and ciphertext
AEAD_TESTS = [

...

💬 maflcko commented on pull request "test: Actually fail when a python unit test fails":
(https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852746032)
Fun fact: This problem would not exit in a type-safe language.

💬 aureleoules commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1852759253)
This pull does fix the crash but I am still getting valgrind errors when executing the benchmark, which did not happen on master before. I haven't verified that these errors were introduced by #25273 but I would suppose so.

```
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
==243376== Memcheck, a memory error detector
==243376== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==243376== Using Valgrind-3.18.1 and LibVEX; rerun with
...

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424551588)
Pushed a functional test covering this case specifically which is rejected due to existence of mempool ancestors.

I'll try and rephrase soon.

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1424554648)
I guess this code will never be hit, so it doesn't matter much. If it was hit, I don't see how the user would shutdown the process without an unclean termination, so might as well do an abort. Generally, it should be fine to abort Bitcoin Core at any time, because any critical data should always be flushed atomically to storage.

But as this code should never be hit, it also seems fine to give the user the option to call RPCs to unload wallets, flush the chainstate, and disconnect peers, etc,
...

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1424568910)
I also don't see that as a real risk, but I do see the problem is that whoever reads this code (or decides to use `ForEachNode` somewhere else) could easily get confused, especially since some callers check again for `fDisconnect`.
I added a refactor commit renaming `ForEachNode()` to `ForEachFullyConnectedNode()` and removing the duplicate checks / comments. What do you think about that approach?

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1424570189)
Good catch, I missed that we are already in blocksonly mode! I moved the added tests to `blocksonly_peer_tests` subtest as suggested.

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1424575234)
I'd agree with adding this warning, but yeah, this PR is not the best place for it.

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-1852816314)
[840a022 ](https://github.com/bitcoin/bitcoin/commit/840a022700910f9ab61e8aff367451dc01a37875)to [bbb01b1](https://github.com/bitcoin/bitcoin/commit/bbb01b19fe02f482a9268f7da62550bff23863e8):
Addressed feedback, and also added a new commit to rename `ForEachNode` and remove duplicate checks, see https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1424568910.

💬 ariard commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1852816465)
> Removing adjusted time from consensus is a big win. Having time samples received from peers be involved in consensus checks is not great and not effective against NTP based attacks (#25908 (comment), #25908 (comment)). The warning itself is not as interesting to me but most of the review on both #25908 and this PR is about the warning and its threshold.

Currently, we're adjusting our local clock from the median of the first 199 outbound peers we're connecting to (only the first 199 due to t
...

✅ maflcko closed an issue: "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed"
(https://github.com/bitcoin/bitcoin/issues/28918)

💬 sipsorcery commented on pull request "Add missing byteswap functions for MSVC":
(https://github.com/bitcoin/bitcoin/pull/29036#issuecomment-1852836454)
Builds fine for me on Windows 11 and msvc v19.38.33130 x64 (Visual Studio 2022).

I ran `bench_bitcoin.exe` but not sure what differences I should see.