💬 fanquake commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2066790159)
Changed to drop the xz changes, and just switch tar.bzip2 tarballs to using tar.gz (same as release tarballs), so we can drop bzip2 as a Guix dep.

> it would be nice to have a script that prints whether the tarball has any differences from the upstream source control it claims to come from.

Sure, we can have a script. Note that the current tarballs will already differ in that sense.

👍 BrandonOdiwuor approved a pull request: "test: Fix intermittent issue in p2p_handshake.py"
(https://github.com/bitcoin/bitcoin/pull/29898#pullrequestreview-2011710275)
Tested ACK 6b02c11d667adff24daf611f9b14815d27963674

💬 maflcko commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1572555381)
I think this could still be racy? For example, the `CNode` may still exist, but the `Peer` may have been deleted already, in which case no peer info is returned, but the `CNode` would still cause the test to crash?

💬 mzumsande commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1572574834)
Are you sure? `DisconnectNodes()` deletes the node first from `m_nodes` (pushing an entry to `m_nodes_disconnected`), and only after that deletes the Peer (in `FinalizeNode()`), so I can't see how we can have a `CNode` but no `Peer`.

💬 cdecker commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2066858884)
> I guess I should add that I don't think the nodes returned by "seed.bitcoinstats.com" are good.
>
> My assumptions is that it's just caching all the nodes it's previously seen and not checking if they're still providing all the services it previously has.

Indeed, I am checking the infrastructure now. And it.look like the crawler died. I'll see if I can get it back with reasonable effort.

💬 stickies-v commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066874947)
I think this is an interesting approach to consider. I'm quite fond of OpenAPI but I'm not sure how well it would lend itself to a JSON-RPC, but perhaps it works well enough? There is a sibling project specifically focused on JSON-RPC (https://open-rpc.org/ ) but it has less tooling and support.

Would be cool if this means we could be spec-driven, build a tool that generates (server) header files from the spec so the RPC method implementations have compile time checks on parameters and respo
...

✅ glozow closed a pull request: "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)"
(https://github.com/bitcoin/bitcoin/pull/29827)

💬 glozow commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#issuecomment-2066929764)
merged, I think there is a github error https://github.com/bitcoin/bitcoin/commit/67c0d93982ad214f5e0c9509e9dc3d6d792ad97c

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572632242)
nit: Any reason not to just wrap the args in `std::move`? Seems like the more intuitive approach, and probably slightly more performant?

```suggestion
std::vector<util::Result<SelectionResult>> results {
std::move(result_srd), std::move(result_knapsack), std::move(result_bnb)
};
```

🤔 stickies-v reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2011883406)
Concept ACK, breaking it out in a smaller PR seems like a good idea, thanks.

I'm unsure about removing the assignment operator altogether. I think it's good to be careful about footguns, but I'm worried about making the interface overly unintuitive.

Would it make sense to only merge another `Result` when using the `Merge` (currently called `Update`) method, and use the assignment operator to overwrite the current result? I think when people want to merge results, they'll probably want to b
...

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572639146)
nit: nice, but maybe best to clean up all 3 instances in one go?

<details>
<summary>git diff on 974b16b258</summary>

```diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 8b62cddb02..9a7e166e68 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -868,7 +868,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
if (total_amount + total_unconf_long_chain > value_to_select) {
return util::Error{
...

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572648440)
`Update` is a bit of an ambiguous name imo, it doesn't clearly indicate if it means replacing or merging into the existing result. I think for this commit, where we can only overwrite the existing result, I think `Replace` would be better. For the next commits in #25665 where we actually add the merging functionality, I think `Merge` or `Combine` would be a better choice.

💬 pinheadmz commented on pull request "rename bitcoin.conf in dist tarball":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2066969773)
If we change the name of the file, I think we should also include a dummy-proof instruction to *remove* `.example` when deploying the file, here:

https://github.com/bitcoin/bitcoin/blob/67c0d93982ad214f5e0c9509e9dc3d6d792ad97c/contrib/devtools/gen-bitcoin-conf.sh#L40-L42

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2066969764)
The "two many replacements" condition is only triggered when a large number of transactions are replaced at once. OpenTimestamps doesn't do that.

On April 19, 2024 8:31:20 AM EDT, levantah ***@***.***> wrote:
>> Possibly the best estimate we can get for that is to look at full-RBF replacements that do not get mined. It's not too uncommon to see, for example, OpenTimestamp's full-RBF replacements mysteriously get mined at the second or third highest fee-rate they were propagated at even though
...

📝 instagibbs opened a pull request: "test: Don't fetch orphan parent when parent is in orphanage"
(https://github.com/bitcoin/bitcoin/pull/29915)
Takes a comment to that effect and makes an assertion.

⚠️ iotamega opened an issue: "Serious Crashes in v27 on Windows 10 and 11"
(https://github.com/bitcoin/bitcoin/issues/29916)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Both the pre-built binaries and manually compiling from source give the same issues with v27.

Very serious crashes that entirely lockup the client and require a force restart across both Windows 10 and Windows 11 across multiple machines. The issue does not happen on my Ubuntu machines and it seems to only be happening with my Windows based machines.

Have tried opening this ticket ma
...

💬 iotamega commented on issue "Serious Crashes in v27 on Windows 10 and 11":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2067175991)
2024-04-19T18:42:06Z Bitcoin Core version v27.0.0 (release build)
2024-04-19T18:42:06Z Qt 5.15.11 (static), plugin=windows (static)
2024-04-19T18:42:06Z Static plugins:
2024-04-19T18:42:06Z QWindowsIntegrationPlugin, version 331520
2024-04-19T18:42:06Z QWindowsVistaStylePlugin, version 331520
2024-04-19T18:42:06Z Style: windowsvista / QWindowsVistaStyle
2024-04-19T18:42:06Z System: Windows 10 Version 2009, x86_64-little_endian-llp64
2024-04-19T18:42:06Z Screen: \\.\DISPLAY1 1600x900, pi
...

💬 iotamega commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2067189094)
> Any v2only option should apply only to IPv4 and IPv6 peers, but not to Tor, I2P, CJDNS because the latter already have (stronger) MITM prevention and encryption.

Agreed on this point regarding Tor, I2P, and CJDNS.

> ## Per-address v2only option
> Surely one would set such an option for an address of a peer already known to support v2. For such a peer, v2 connection would already be established currently. So what is the point of having such an option? To make sure MITM does not interfere
...

👍 pinheadmz approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2012295896)
ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62

Reviewed each commit. Ran all unit and functional tests on arm/macos and then AGAIN on arm/linux (docker) when I noticed the relevant test requires it :-P

Confirmed the tests fail on master without the patch.

Also confirmed the "crash on port collision" behavior with my own test, @vasild take it or leave it: https://gist.github.com/pinheadmz/0bea4bf8d8bf9af85f8ae326286d3082

<details><summary>Show Signature</summary>

```
-----BEGIN PGP
...

💬 instagibbs commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#issuecomment-2067244164)
Seeing a failure of this test in my other PR, though I can't make heads or tails of the failure: https://cirrus-ci.com/task/6740372997537792