💬 TheCharlatan commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1584865007)
I'm surprised we never had to match the magic bytes to a chaintype. Instead of defining new strings here though, could you use `ChainTypeToString(ChainType::MAIN)`?

💬 TheCharlatan commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1584868473)
I think this is fine where it is, I like keeping these single type utilities small.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584882538)
Ah, just found a mistake. I think this can go in a followup though.
```suggestion
if (rejected_parent_reconsiderable.has_value() && *rejected_parent_reconsiderable != parent_txid.ToUint256()) {
```

👍 TheCharlatan approved a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2031473523)
ACK 9552619961049d7673f84c40917b0385be70b782

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584890852)
Hmm, we're looping over unique parents already, so I think this code should be fine? In which case is there a bug?

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584892625)
they're unique txids, which scenarios will this fix?

✅ fanquake closed a pull request: "depends: fix Qt to not use `-q` with `ar`"
(https://github.com/bitcoin/bitcoin/pull/30004)

🤔 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