💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244442031)
Rather than including everything, it might be good to have a separate compilation unit for code that needs to be in `util/setup_common`, versus the current `util/net` code that is only needed for a handful of the tests.

🚀 ryanofsky merged a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914)

👍 ryanofsky approved a pull request: "kernel: Add interrupt class"
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1502027676)
Code review ACK 001acf37dff40688d82c3b1a7ff9ed3901addb33. This looks very good, and hopefully can be merged soon with more review.

To encourage review, I would maybe rewrite the PR title and description to be a little less abstract, and describe the benefits concretely. For title, would suggest "kernel: Remove all `ShutdownRequested` and `AbortNode` calls from validation code." For description would suggest: "Replace all `AbortNode` calls in validation code with new fatal error and flush erro
...

💬 ryanofsky commented on pull request "Disable and uncheck blank when private keys are disabled":
(https://github.com/bitcoin-core/gui/pull/739#issuecomment-1610420201)
@S3RK do you think the UI changes here are actually worse, or just not an improvement? I agree the changes aren't ideal and aren't a clear win, but I think they don't make it worse necessarily. This PR disables the "make blank wallet" checkbox when "disable private keys" checkbox is enabled, instead of just automatically checking it. I think disabling the checkbox makes sense because it makes it clear than once private keys are disabled, the choice of whether to have a blank wallet is moot becau
...

💬 miketwenty1 commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1610431814)
@Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.

💬 evansmj commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1244653007)
What do you think of renaming the `TxStateConflicted` struct to `TxStateBlockConflicted`, since that one is used for block conflicts and there is now a difference here between block and mempool conflicts?

`//! State of rejected transaction that conflicts with a confirmed block.
struct TxStateConflicted {
...
};`

👍 stratospher approved a pull request: "Introduce secp256k1 module with field and group classes to test framework"
(https://github.com/bitcoin/bitcoin/pull/26222#pullrequestreview-1502244056)
tested ACK d4fb58a. really liked how this PR makes the secp256k1 code in the tests more intuitive and easier to follow!

✅ MarcoFalke closed an issue: "An option for a shell command that runs just before bitcoind completes shutting down."
(https://github.com/bitcoin/bitcoin/issues/27984)

💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1610800583)
Someone on Twitter drew attention to potential security implications, for instance:
- [New ZIP domains spark debate among cybersecurity experts](https://www.bleepingcomputer.com/news/security/new-zip-domains-spark-debate-among-cybersecurity-experts/)
- [Don't Click That ZIP File! Phishers Weaponizing .ZIP Domains to Trick Victims](https://thehackernews.com/2023/05/dont-click-that-zip-file-phishers.html)

💬 MarcoFalke commented on issue "Add maxrelaytxfee":
(https://github.com/bitcoin/bitcoin/issues/27983#issuecomment-1610811473)
I don't understand this. If the inputs are signed by someone other than the miner, they may (or should) have broadcast the transaction already, making this setting useless. Also, once it is included in a block, the transaction can be fee-sniped by another miner in a re-org.
And obviously implementing the setting as suggested by you will also apply it to network transactions, which doesn't make sense either.

💬 S3RK commented on pull request "Disable and uncheck blank when private keys are disabled":
(https://github.com/bitcoin-core/gui/pull/739#issuecomment-1610881553)
@ryanofsky no no, I don't think this change is making it worse.

From my perspective, it's a in improvement for core developers, because it provides clarity on the wallet flags usage.
UX wise it's ~0 as the flags are implementation details. Both pre- and post-PR whether blank checkbox is disabled or automatically checked is equally not explained to the users.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244795380)
I tried extracting the mocked sockets out from `test/util/net.h` into a separate `test/util/mocked_sockets.h` and then compared the re-compilation time if that is modified (e.g. something in `DynSock` declaration is changed). In both cases recompilation takes ~28 seconds, so it makes no difference (in practice).

Here is the change (on top of this PR):

<details>
<summary>mocked_sockets.h</summary>

```diff
commit 398a2d5717b6ef6760e48eac92aa92127157a67a (HEAD -> e2e_tests)
Parent: 4557
...

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244823822)
(No opinion on the change), just

> In both cases recompilation takes ~28 seconds

Seems odd that re-compilation of all tests takes only 28 seconds when `mocked_sockets.h` is modified, which is included in `setup_common.h`, which is included in all test files, no?

👍 Dezzj approved a pull request: "Tests: Fill out dust limit unit test for known types except bare multisig"
(https://github.com/bitcoin/bitcoin/pull/26875#pullrequestreview-1502509586)
Approved

📝 vasild opened a pull request: "test: remove race in the user-agent reception check"
(https://github.com/bitcoin/bitcoin/pull/27986)
In `add_p2p_connection()` we connect to `bitcoind` from the Python client and check that it has received our version string.

This check looked up the last/newest entry from `getpeerinfo` RPC, assuming that it must be the connection we have just opened. But this will not be the case if a new inbound or outbound connection is made to/from `bitcoind` in the meantime.

Instead of the last entry in `getpeerinfo`, check all and find the one which corresponds to our connection using our outgoing a
...

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1610998377)
I stumbled on this playing with https://github.com/bitcoin/bitcoin/pull/27509:

https://cirrus-ci.com/task/6722465808777216?logs=ci#L8761-L8771

<details>
<summary>CI log</summary>

```
2023-06-27T09:50:26.796000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/tmp/cirrus-ci-build/ci/scratc
...

💬 MarcoFalke commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1244887087)
Why would this ever be not found, given that the previously the connection was sync_with_ping'd?

In any case, you can use `next()` to do the assertion. Roughly:

```py
next([p if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}" p in self.getpeerinfo()])
assert_equal(p["subver"], P2P_SUBVERSION)

💬 willcl-ark commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1611033313)
> I get this when compiling with both gcc and clang-16. I am also using libevent 2.1.12. Is there anything unique about your setup @willcl-ark ? Also, did you Ctrl-C the `waitforblockheight` command before trying to stop bitcoind?

I've had more reports of this deadlock so come back to try and repro it again, but don't seem to be able to no matter what I try 🤷🏼‍♂️

I am on Ubuntu 23.04, also using Clang 16 and libevent 2.1.12-stable-8ubuntu3. It doesn't look like there are any debian patc
...

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244922368)
Compiling everything from scratch (after `git clean -fdx`) takes about 1m 30sec (including autogen and ./configure which are single-threaded). The CPU on the machine is:

> AMD Ryzen 9 7950X 16-Core Processor (4491.70-MHz K8-class CPU)

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244937645)
For a useful benchmark, you'll have to measure the total CPU time, not the CPU time of just the translation unit that takes the longest.

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1244943695)
> Why would this ever be not found

Well, I do not know, maybe some typo in `p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"` or some other unexpected thing. That is just another sanity check.

Taking a step back I wonder if this check as much value - why would `bitcoind` not have received our user agent string? I wouldn't object if somebody wants to drop the check altogether.

> p=next([p if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}" p in self.getpeerinfo()])

I find this compl
...