🤔 mzumsande reviewed a pull request: "test: Fix intermittent issue in p2p_handshake.py"
(https://github.com/bitcoin/bitcoin/pull/29898#pullrequestreview-2011586560)
Tested ACK 6b02c11d667adff24daf611f9b14815d27963674

💬 maflcko commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066720293)
> https://bitcoincore.org/en/doc/

Yes, I am aware. They are based on the `help` RPC reply. However, they are not machine readable, at least not in a standard format, so I don't think it helps to solve the issues highlighted in the original post.

💬 sipa commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066724167)
I vaguely recall an issue or discussion around machine-readable APIs in the past, but I cannot find it. I believe @laanwj commented on it.

💬 maflcko commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066731848)
I can only recall the gRPC (https://github.com/bitcoin/bitcoin/issues/16719) and CBOR (https://github.com/bitcoin/bitcoin/issues/22866) discussions, but those are serialization related, not about a standardized machine-readable interface documentation and specification.

💬 maflcko commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2066751093)
The current set of the changes look fine. However, to actually review all 10 changed packages, 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.

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1572504707)
duplicate log?

⚠️ maflcko opened an issue: "RFC: In guix compile the GUI sequentially from everything else?"
(https://github.com/bitcoin/bitcoin/issues/29914)
Compiling the GUI pulls in quite a few dependencies, which could theoretically include backdoors that are leaked into bitcoind (or other non-GUI utils) as well.

A possible mitigation would be to compile the GUI in a separate guix container from the rest of the binaries. The downside would be that the node library, and the `depends` dependencies of the node library would have to be compiled twice, but the overhead may be worth it?

(I won't be working on this, but I wanted to keep track of t
...

💬 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