💬 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

💬 hebasto commented on issue "Serious Crashes in v27 on Windows 10 and 11":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2067254672)
@iotamega

> Right click on a bad actor peer and ban them, immediate lockup across MULTIPLE systems and both Windows 10 and Windows 11.

I'll try to reproduce the issue. Meanwhile, would you please be more specific regarding

> ### How did you obtain Bitcoin Core
>
> Other

If you're using pre-built binaries, where they were downloaded from?

💬 iotamega commented on issue "Serious Crashes in v27 on Windows 10 and 11":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2067260215)
> If you're using pre-built binaries, where they were downloaded from?

I downloaded them directly from https://bitcoincore.org/bin/bitcoin-core-27.0/

I also thought maybe it was something with those so I then compiled on my own as well using WSL and encountered the same issue. Both give the same error, the prebuilt binary was also blocked by Windows Defender in the latest build of Windows 11 for containing a "keylogger" so I whitelisted it in Defender but haven't seen an issue like that in
...

💬 hebasto commented on issue "Serious Crashes in v27 on Windows 10 and 11":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2067262898)
> It doesn't happen 100% of the time so it might take a while to reproduce, it is a strange one.

Then I suggest to remove the word "Serious" from the issue title :)

💬 iotamega commented on issue "Serious Crashes in v27 on Windows 10 and 11":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2067264471)
> > It doesn't happen 100% of the time so it might take a while to reproduce, it is a strange one.
>
> Then I suggest to remove the word "Serious" from the issue title :)

I can do that but it pretty serious when it requires a restart every few hours because I have to ban a very serious bad actor from causing a massive DDOS against my node, but I will remove it for the time being. I imagine others will be reporting the same issue.