💬 ryanofsky commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1294879351)
In commit "First draft of background sync logic" (6be9fd22ee2b9759cdd30a7bca4d07a9346eb57b)

This is an exaggeration, right? Peer doesn't seem completely useless if it doesn't contain the snapshot block because it could contain blocks leading up the snapshot block. It this is right it would be good to clarify, or maybe change to allow requesting blocks from the peer.

💬 MarcoFalke commented on pull request " ci: Fix macOS-cross SDK rsync":
(https://github.com/bitcoin/bitcoin/pull/28273#issuecomment-1679303820)
Changed the fix and added an explanation. Let's hope this works. I presume this was a silent merge conflict between 80d70cb6b04b5a3c13e661c0718a4b5108d55869 and 85e672ab3dfbbeec5255befcbcd239a11536be0e. cc @hebasto

💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1294891951)
Good idea, simpler and faster. Done (thanks!)

💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1294893286)
I got a crash after 7.4M attempts:

fuzz: ../../src/wallet/test/fuzz/coinselection.cpp:120: void wallet::coinselection_fuzz_target(FuzzBufferType): Assertion `result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed.`

You should be able to replicate the issue with:

$ echo "MP3//////wT/LzMBEABL////////Wv///yWyEABLAADoQP//PP//CAAAPQAAAAAAAAAAPQEAAAAA AAAA/V12+w==" | base64 -d > crash.input
$ FUZZ=coinselection src/test/fuzz/fuzz crash.input

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1294895356)
When one thread waits for another to execute a _particular line of code_, if that line of code is not executed for some time the waiting thread has to assume a timeout failure. I.e. some waiting is unavoidable, I am leaving the code as it is.

Do you consider this resolved?

💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679315436)
The fuzz test crashed for me after 7.4M attempts:

fuzz: ../../src/wallet/test/fuzz/coinselection.cpp:120: void wallet::coinselection_fuzz_target(FuzzBufferType): Assertion `result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed.`

You should be able to replicate the issue with:

```
$ echo "MP3//////wT/LzMBEABL////////Wv///yWyEABLAADoQP//PP//CAAAPQAAAAAAAAAAPQEAAAAA AAAA/V12+w==" | base64 -d > crash.input
$ FUZZ=coinselection src/test/fuzz/fuzz crash.i
...

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1294895367)
Maybe add a comment linking to https://github.com/github/roadmap/issues/528 to say that this should be used, when available?

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1294901116)
Added a comment.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1679324809)
`55c84c2d3b...ca7a9983eb`: add a comment in the source code, as per https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269953546

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1294906169)
ConsumeIntegral can return 0, so this won't be in the future. Can just drop ConsumeIntegral completely?

💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1294921322)
Thanks, Fix this and the nit in 530ea157ea1c25821711617e5f1da3e95cfdc66d

💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#issuecomment-1679353180)
> Due to the multiple datadirs in the `feature_config_args` test, this assertion actually _does_ fail in `combine_logs.py`:
>
> https://github.com/bitcoin/bitcoin/blob/db7120fbaddff6037734d8e42d1d002d773f8567/test/functional/combine_logs.py#L86
>
> I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.

@pinheadmz The update on 530ea157ea1c25821711617e5f1da3e95cfdc66d
is using the `node[0]`'s `bitcoin.conf` to set the option
...

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679360517)
@murchandamus I got a crash with 5000 as well, perhaps we should increase the value even more.

💬 MarcoFalke commented on pull request "ci: Fix macOS-cross SDK rsync":
(https://github.com/bitcoin/bitcoin/pull/28273#issuecomment-1679380245)
Still fails:

```
clang: warning: no such sysroot directory: '/tmp/cirrus-ci-build/depends/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers' [-Wmissing-sysroot]
ld: library not found for -lc++
clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

https://cirrus-ci.com/task/6482314977345536?logs=ci#L676

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1679417885)
> Approach ACK. I wouldn't say the API here is perfectly ideal because it doesn't check everything at compile time that could theoretically be checked at compile time.

I don't think any API changes will be required, assuming C++20 const(expr/init/eval), to turn run-time checks into compile-time checks, but I haven't confirmed this.

💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679417244)
I don’t think that an absolute value will work. My guess is that the crash may be caused by `m_cost_of_change` being generated randomly independent from `m_change_fee`. Usually, `cost_of_change` cannot ever be smaller than `m_change_fee`, so if it happens to be smaller here, it may interfere with the change calculation?

I attempted to calculate `cost_of_change` instead from `m_change_fee + input_size×LTFRE`, leaving the fuzz input consumption unchanged. It made the test pass:

```diff
@@
...

💬 achow101 commented on pull request "test: check backup from `migratewallet` can be successfully restored":
(https://github.com/bitcoin/bitcoin/pull/28257#issuecomment-1679485865)
ACK 769f5b15f207ce6d1067672ea5e195541c97de6b

💬 achow101 commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1295036091)
I think the since the libevent structs in this file are only pointers, the actual definitions aren't needed hence the include is not strictly necessary?

💬 theuni commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1295048904)
Yeah, iirc everything in libevent can be forward-declared except for `evutil_socket_t`. It's very annoying.

Because everything else is already forward-declared as @achow101 mentioned, only the definition of `evutil_socket_t` is needed. `evutil_socket_t` is actually defined in `event2/util.h` (note that `event.h` includes `util.h`.

So I agree with this change. It's a small reduction in needed includes.

💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1679547504)
Updated for @vasild's review feedback to change `m_added_nodes` from a vector to an unordered set (https://github.com/bitcoin/bitcoin/pull/28248/commits/3f4a1f0bc98159f86d910227fd57d05d10bb8889), which simplifies and speeds up the `AddNode()` and `RemoveAddedNode()` methods along with the commit that follows it, `p2p: do not make automatic outbound connections to addnode peers` (048e1af).