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

💬 furszy commented on issue "[bench] Assertion error on wallet_create_tx.cpp, line 133.":
(https://github.com/bitcoin/bitcoin/issues/29069#issuecomment-1853153904)
See #29065.

💬 kevkevinpal commented on issue "[bench] Assertion error on wallet_create_tx.cpp, line 133.":
(https://github.com/bitcoin/bitcoin/issues/29069#issuecomment-1853169756)
> See #29065.

thank you will pull and review the PR!

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

closes https://github.com/bitcoin/bitcoin/issues/29069

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1853174830)
Force pushed suggestion by @furszy (thanks!) https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1424023270

💬 0xakihiko commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1853335872)
> 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)

Because that's how we fix bugs. One line commits, broken test cases, broken ecosystems and a totalitarian view of discussions.

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