🤔 dergoegge reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2031444770)
light Code review ACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

💬 dergoegge commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584871688)
I think attempting to accept a 1p1c package could also be done when reconsidering transactions from the orphanage in `ProcessOrphanTx`. E.g. you might have a 1p1c package in your orphanage (P -> C) and if P is reconsidered but rejected as `TX_RECONSIDERABLE` you might want to look for C in the orphanage and if found try 1p1c acceptance.

Maybe its not worth adding right now (any scenarios I can think of seem somewhat rare) but perhaps worthwhile later if the data suggests so.

💬 dergoegge commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584890259)
Why is this sort comparison different from the one in `GetChildrenFromSamePeer`?

These functions are almost identical as well. Perhaps a more generic ```std::vector<std::pair<CTransactionRef, NodeId>> GetChildren(const CTransactionRef&)``` that returns all children and their senders would be less code?

💬 ryanofsky commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584900889)
> I think this means the positional `request.params` accessor, so this is better to leave as-is, no?

I think this was called `check_positional` because it was calling the `Arg(size_t)` function and the the check below was called `check_named` because it was calling the `Arg(string_view)` function. So now that the check below is deleted and this now calling `Arg(string_view)`, it would make more sense to call this `check_named` than `check_positional`.

But the point of this is really to cal
...

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584902362)
Ah you're right, nevermind!

👍 dergoegge approved a pull request: "msvc: Compile `test\fuzz\bitdeque.cpp`"
(https://github.com/bitcoin/bitcoin/pull/29983#pullrequestreview-2031510928)
utACK 774359b4a96d2724dc70f900cb71e084a77164da

💬 stickies-v commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584915042)
> I think this was called `check_positional` because ...

Indeed, that was the rationale.

> so maybe a name like `check_args`

or `check_equivalence`? (no big deal either way ofc)

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2031518836)
Updated 9552619961049d7673f84c40917b0385be70b782 -> 6a8b2befeab25e4e92d8e947a23e78014695e06c ([`pr/saferesult.8`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.8) -> [`pr/saferesult.9`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.8..pr/saferesult.9)) tweaking variables to avoid shadowing

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1584914512)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581580725

> I'm happy to go with any suggestion. I don't see shadowing as a problem in all cases, so this is just the best way to write the code that I could think of.

Not sure why I couldn't think of a good way to write this without shadowing last week. But added `_retry` suffixes now so the code should be clearer and shadowing should be gone :sunglasses:

💬 glozow commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584921222)
Should this be happening when `duplicate_inputs` is true (we'd be intentionally re-adding copies of the inputs we used)? Maybe I'm interpreting `duplicate_inputs` incorrectly but I thought that just means "we're ok with duplicates" not "we will add a copy of an outpoint every time we use it"

💬 glozow commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584925110)
Approach-wise, sorry to bikeshed, but did you consider just making outpoints a set instead of a vector?

💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584932568)
I am happy to review a follow-up, if there is a better name. But I think the current name is perfectly fine and accurate.

In any case, if the legacy code is removed, the test will go away as well.

💬 sr-gi commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584955259)
Yeah, that may reduce the boilerplate and, given this is a test, performance shouldn't matter much

💬 sr-gi commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584957576)
The way I've understood `duplicate_inputs` is: "We are OK with using the same input more than once in a transaction". The way the test was built, re-using inputs across transactions was always an option, so I took that for granted.

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2085563710)
re-ACK 6a8b2befeab25e4e92d8e947a23e78014695e06c

✅ glozow closed a pull request: "txorphanage: add size accounting, use wtxids, support multiple announcers"
(https://github.com/bitcoin/bitcoin/pull/28481)

💬 glozow commented on pull request "txorphanage: add size accounting, use wtxids, support multiple announcers":
(https://github.com/bitcoin/bitcoin/pull/28481#issuecomment-2085578562)
Closing for now. The first commit is superseded by #30000, and the rest will come after `TxDownloadManager` refactoring

🤔 furszy reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2031664498)
re-ACK https://github.com/bitcoin/bitcoin/commit/6a8b2befeab25e4e92d8e947a23e78014695e06c

💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2085584460)
@hebasto thanks for the review, I addressed your comments

💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2085586513)
> > > I think most of the criticism of this PR from AJ is saying that the changes here are unnecessary, not really that they are harmful,

> > I said "As I've already said, I think `m_log.SetCategory(NET); m_log.Debug("connected to foo")` is much worse than `LogDebug(NET, "connected to foo")`." Does that really not come across as me saying they're harmful? I really don't think I'm being **that** unclear. Anyway, I'll bow out, I'm already sorry for touching this.

> I think it's possible you
...