✅ achow101 closed an issue: "Outcome of the syscall sandbox experiment"
(https://github.com/bitcoin/bitcoin/issues/24771)

🚀 achow101 merged a pull request: "Remove the syscall sandbox"
(https://github.com/bitcoin/bitcoin/pull/27896)

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1501951643)
Code review ACK 3c83b1d884b419adece95b335b6e956e7459a7ef. Just Marco's suggested error handling fixes since last review

🤔 jonatack reviewed a pull request: "test: add end-to-end tests for CConnman and PeerManager"
(https://github.com/bitcoin/bitcoin/pull/26812#pullrequestreview-1501951235)
Reviewed/built/tested the first four commits up to https://github.com/bitcoin/bitcoin/commit/d2f46c705540c74c2b6f83a66535c3ead1cb95d4.

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244454225)
d2f46c7 Verified the behavior improvement in this commit by changing this line to make it fail.

master

```bash
$ ./src/test/test_bitcoin -t banman_tests
Running 1 test case...
libc++abi: terminating due to uncaught exception of type std::runtime_error: 'Dropping entry with unknown version (2) rom ban list' not found in debug log

******** errors disabling the alternate stack:
#error:22
Invalid argument
unknown location:0: fatal error: in "banman_tests/file": signal: SIGABR
...

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