💬 MarcoFalke commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1643269814)
Nice. lgtm ACK e667bd68a10512ddc784df44bdcb63ee441e5551

🤔 MarcoFalke reviewed a pull request: "test: fix intermittent failure in wallet_resendwallettransactions.py"
(https://github.com/bitcoin/bitcoin/pull/28108#pullrequestreview-1538476086)
left a follow-up idea

💬 MarcoFalke commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#discussion_r1268966663)
Generally I am not really a fan of using `assert_debug_log` to sync. If you do, my preference would be to use the timeout argument. Even better would be to sync inside the body. Either with a call to `syncwithvalidationinterfaceque` or by modifying the RPC:


```diff
diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp
index 3828401642..f614253195 100644
--- a/src/rpc/node.cpp
+++ b/src/rpc/node.cpp
@@ -93,6 +93,7 @@ static RPCHelpMan mockscheduler()
// protect against null pointer der
...

💬 MarcoFalke commented on pull request "util: Show descriptive error messages when FileCommit fails":
(https://github.com/bitcoin/bitcoin/pull/26654#issuecomment-1643358133)
CI can be ignored.

lgtm ACK 5408a55fc87350baeae3a32f1003f956d5533a79 💡

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgt
...

💬 MarcoFalke commented on pull request "refactor: Open file in FileCommit":
(https://github.com/bitcoin/bitcoin/pull/28006#issuecomment-1643368606)
Looks like this will make it safer to remove `std::ftell` when using XOR, so I'll reopen for now as draft.

📝 MarcoFalke reopened a pull request: "refactor: Open file in FileCommit"
(https://github.com/bitcoin/bitcoin/pull/28006)
This improves `FileCommit`: The patch removes the requirement from the call site to open a raw C-style FILE pointer. Now the call site can use any style of file IO.

There are many other smaller improvements, which are explained in each commit.

📝 MarcoFalke converted_to_draft a pull request: "refactor: Open file in FileCommit"
(https://github.com/bitcoin/bitcoin/pull/28006)
This improves `FileCommit`: The patch removes the requirement from the call site to open a raw C-style FILE pointer. Now the call site can use any style of file IO.

There are many other smaller improvements, which are explained in each commit.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269027565)
I will look into replacing this with `Assert()`.

"exception from destructor in c++" is a topic well covered all over the internet. The compiler produces a warning for that. Here is an example:

```cpp
class A
{
public:
~A() {
std::cout << "before throw a\n";
// warning: '~A' has a non-throwing exception specification but can still throw [-Wexceptions]
//171 | throw std::runtime_error("a");
// | ^
throw std::runtime_er
...

💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#issuecomment-1643403050)
Anything left to do for a test-only change with two ACKs or is this rfm?

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269042432)
Yeah, but this being an "Abort trap" is well defined, not "not allowed". And I think the tests aborting on failure is fine.

✅ hebasto closed a pull request: "Add `UNREACHABLE` macro"
(https://github.com/bitcoin/bitcoin/pull/26504)

⚠️ hebasto opened an issue: "build: Windows debug cross-build fails with `-O0`"
(https://github.com/bitcoin/bitcoin/issues/28109)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

When cross-compiling for Windows with the `-O0` optimization level the build fails.

The error can be fixed by the `-Wa,-mbig-obj` assembler flag. However, @theuni [suggested](https://github.com/hebasto/bitcoin/pull/18#discussion_r1267219453):
> We should definitely look into breaking up that object.

### Expected behaviour

The build completes with no error.

### Steps to reproduce

``
...

✅ fanquake closed an issue: "bumpfee behavior with custom change address"
(https://github.com/bitcoin/bitcoin/issues/11233)

✅ fanquake closed an issue: "Privacy Issue - Increase Fee with Custom Change Address grabs new UTXO "
(https://github.com/bitcoin/bitcoin/issues/20795)

🚀 fanquake merged a pull request: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467)

💬 whitslack commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1269177446)
@furszy makes an important point. Consider the case where I am paying someone on chain under the stipulation that *they* will eat the mining fee. (Thus, I specify *their* address in `subtractfeefrom` when I'm constructing my transaction.) Later, they tell me that they can't wait for confirmation any longer and need an urgent fee bump. If I naïvely specify their output as the `reduce_output`, I may end up paying them ***much more*** than I intended to if I only have large UTxOs in my wallet.

🚀 fanquake merged a pull request: "fuzz: Generate process_message targets individually"
(https://github.com/bitcoin/bitcoin/pull/28066)

📝 fanquake opened a pull request: "doc: correct Fedora systemtap dep"
(https://github.com/bitcoin/bitcoin/pull/28110)

⚠️ hebasto opened an issue: ""missing operand" assembler warnings on Windows"
(https://github.com/bitcoin/bitcoin/issues/28111)
On the master branch @673acab223c0f896767b1ae784659df9f95452ae on Ubuntu 23.04:

```
$ make -C depends HOST=x86_64-w64-mingw32 NO_QT=1 DEBUG=1 NO_HARDEN=1
$ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
$ make
...
CXX crypto/libbitcoin_crypto_base_la-aes.lo
CXX crypto/libbitcoin_crypto_base_la-chacha_poly_aead.lo
CXX crypto/libbitcoin_crypto_base_la-chacha20.lo
CXX crypto/libbitcoin_crypto_base_la-hkdf_sha256_32.lo
CXX cry
...

🤔 stickies-v reviewed a pull request: "rfc: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107#pullrequestreview-1538994126)
Concept ACK.

We don't consistently have clear variable naming (e.g. the ambiguous `hash`; or `txid` being used to refer to a wtxid), sometimes requiring quite a bit of developer time to figure out the expected ~type. I have experienced this quite a bit myself.

Introducing types makes this explicit at relatively low overhead, and I think the approach of subclassing `uint256` makes sense.

We'll have to be careful for this not to lead to unnecessary refactoring code churn, but that shouldn
...