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

💬 luke-jr commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1853037139)
Can you move the off-topic chatter somewhere else? This PR is strictly a bugfix for existing functionality. Whether you agree or disagree with that functionality is irrelevant.

P.S. [OP_RETURN was originally for sending coins to miners](https://buildingbitcoin.org/bitcoin-dev/log-2012-08-20.html#L-1512)

👍 theStack approved a pull request: "test: Actually fail when a python unit test fails"
(https://github.com/bitcoin/bitcoin/pull/29068#pullrequestreview-1778639038)
ACK fa0534d7e47d44428d3f9dea6d2f6b8e86df22d4

⚠️ kevkevinpal opened an issue: "[bench] Assertion error on wallet_create_tx.cpp, line 133."
(https://github.com/bitcoin/bitcoin/issues/29069)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

On bitcoin/master d646ca35d991e4f694096fdbd2d2ebd8cebf244e
I compiled bitcoin core with bench on macOS and tried to run the benchmarks but got an error as such
`Assertion failed: (tx_res), function operator(), file wallet_create_tx.cpp, line 133.`

this is the last few logs I see before it fails
```
./src/bench/bench_bitcoin

| ns/op | op/s | err% |
...

💬 kevkevinpal commented on issue "[bench] Assertion error on wallet_create_tx.cpp, line 133.":
(https://github.com/bitcoin/bitcoin/issues/29069#issuecomment-1853128008)
Seems like this one specifically is giving my machine trouble
`./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection`