💬 achow101 commented on pull request "Doc: add a comment referencing past vulnerability next to where it was fixed":
(https://github.com/bitcoin/bitcoin/pull/30538#issuecomment-2357146939)
~0

I'm not sure that having a comment that references code that no longer exists is all that useful.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1764183486)
txrequest and the bloom filters take uint256s, so this seemed easiest. I don't feel strongly. `RelayTransaction` isn't called here...?

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2357211750)
> Ran txdownloadman_impl and it crashed on this assertion:

> I think it'd make sense for ReceivedTx to abort trying to find a package if the parent is in reject filter, even if also in recent reject filter.

Thanks @marcofleon @instagibbs! Was able to reproduce the crash.

I hadn't noticed that quirk about the nested `if` conditions before - we were kind of expecting that a tx wouldn't have multiple things wrong with it. I agree it makes more sense to quit on anything that's non-reconside
...

💬 davidgumberg commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1764221454)
nit:
```suggestion
LogInfo("Peer is stalling block download, %s\n", pto->DisconnectMsg(fLogIPs));
```

💬 glozow commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1764225430)
Is the hope to be able to reconstruct direct parents of a transaction from the txgraph? Even if a direct parent is also an indirect ancestor?

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1764228793)
thank you! Addressed this in [bc4a929](https://github.com/bitcoin/bitcoin/pull/30875/commits/bc4a929cd716cd2b412c70754749d4fda0ca2a10)

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1764229116)
No, that's impossible (A->B->C is indistinguishable from (A->B->C,A->C)); the goal is just computing a set of dependencies whose corresponding ancestry matches the DepGraph.

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1764229313)
thank you! Addressed this in [bc4a929](https://github.com/bitcoin/bitcoin/pull/30875/commits/bc4a929cd716cd2b412c70754749d4fda0ca2a10)

I will take a look at the backtick nit, to see if I can find already existing instances of `cmake` with no backticks

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1764229340)
thank you! Addressed this in [bc4a929](https://github.com/bitcoin/bitcoin/pull/30875/commits/bc4a929cd716cd2b412c70754749d4fda0ca2a10)

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2357229647)
> Concept ACK
>
> Nit: could also fix
>
> ```diff
> --- a/test/util/test_runner.py
> +++ b/test/util/test_runner.py
> @@ -5,7 +5,7 @@
> # file COPYING or http://www.opensource.org/licenses/mit-license.php.
> """Test framework for bitcoin utils.
>
> -Runs automatically during `make check`.
> +Runs automatically during `ctest --test-dir build/`.
>
> Can also be run manually."""
> ```

thank you! Addressed this in [bc4a929](https://github.com/bitcoin/bitcoin/pull/30875/commi
...

💬 davidgumberg commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2357248387)
ACK https://github.com/bitcoin/bitcoin/commit/bc4a929cd716cd2b412c70754749d4fda0ca2a10

pico-nit: your branch messes up the 80 character wrapping in a few spots in `doc/developer-notes.md`. This is not really consistent through the docs anyways, so I don't mind personally.

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1764229521)
Maybe self-explanatory :sweat_smile:

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1764236837)
I was briefly confused about ordering stability, and then realized this has length 1

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1764231050)
maybe "bytes" instead? to make it hard to confuse with vsize

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1764230149)
Perhaps `GetOrphanTransactions`, since this is on the peerman and orphan blocks can exist too

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1764231219)
mention BIP 141?

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1764235865)
Maybe should have a method for testing that a tx isn't in orphanage so we can test that here

💬 glozow commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1764240141)
thought so... how is it intended to be used?

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2357254842)
@glozow It's already used in two places in this PR.

Maybe I should clarify in the PR description that this is just a generic utility feature, not something the next layer up will need per se (AFAIK). But because I was introducing a second place where it's invoked, and it's already used somewhere in the test, I felt it made sense to turn it into a proper DepGraph function, with proper tests.

💬 davidgumberg commented on something "":
(https://github.com/bitcoin/bitcoin/commit/32680702808557517c77507e9f42a1bcf94a7ab2#r146852995)
I believe your PR makes sure that `DisconnectMsg` gets printed any time `fDisconnect == true` so this line results in a duplicate log in each of those instances.

For example when doing `bitcoin-cli setnetworkactive false`:

```console
2024-09-18T00:32:49Z SetNetworkActive: false
2024-09-18T00:32:49Z [net] Network not active, disconnecting peer=0 peeraddr=xx.xx.xx.xx:8333
2024-09-18T00:32:49Z [net] Network not active, disconnecting peer=1 peeraddr=xx.xx.xx.xx:8333
2024-09-18T00:32:49Z [n
...