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

💬 maflcko commented on pull request "Add missing byteswap functions for MSVC":
(https://github.com/bitcoin/bitcoin/pull/29036#issuecomment-1852854218)
See https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841125275 for benches that could make a difference.

But this requires a rebase on master first, see

> Could rebase on master now that https://github.com/bitcoin/bitcoin/pull/29045 has gone in.

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1424611283)
Doesn't this leave an unsafe dangling reference if we don't have a variable capturing ownership of the `unique_ptr`?

💬 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_r1424611995)
I like that name! I missed your comment in the most recent push, but will change it tomorrow!

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1424659497)
This defeats the purpose of this pull because it generates a randomly named data directory path; the point of this PR is to have a fixed datadir pathname so that I can have various files open in an editor across multiple test runs (not have to determine files' locations and re-open them on each test run).

> Be forced to place the test only inside the temp directory doesn't seems so useful to me.

Well, if we want the datadir pathnames to be static (which is what _I'm_ trying to accomplish w
...