💬 sipa commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1621949444)
Rebased and switched from `bool` to an `enum class FuzzResult` which has values `MAYBE_INTERESTING` and `UNINTERESTING`, making it hopefully clearer what the return values correspond to.

📝 glozow opened a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031)
Milestone 1 of package relay p2p changes. See #27463 for full project tracking.

Please see #27742 for how this PR fits into the big picture. I strongly suggest that reviewers look at that PR first to decide if they are comfortable with the overall approach.

This PR is mainly refactors, with a few behavior changes and improvements:
- Introduces `TxPackageTracker`, which starts out as a wrapper for the `TxOrphanage`. This will be the main "package relay" module as well.
- Adds a new log ca
...

🤔 glozow reviewed a pull request: "[NO MERGE] BIP331 Ancestor Package Relay"
(https://github.com/bitcoin/bitcoin/pull/27742#pullrequestreview-1514439276)
Opened #28031 for milestone 1, addressing the comments here pertaining to those commits.

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253067629)
As in `Announcement::m_is_wtxid`, sorry. Clarified (in other PR).

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253060069)
Done (in other PR)

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253138929)
Changed/clarified (in other PR)

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253139157)
Added param comment (in other PR)

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253068219)
Removed (in other PR)

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253066804)
Ah I mean of the `GenTxid`. Clarified comment (in other PR), thanks.

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253243468)
I've squashed the commits so that it's more clear that this was copied to TxPackageTracker (other PR)

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253253635)
Done (in other PR)

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253252501)
Done (in other PR)

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253290488)
Fixed here

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253291132)
Done (in other PR)

💬 ajtowns commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1253319265)
I'm not sure what I was trying to suggest in the comment above: having `BeginTime()` return a height doesn't make sense. Maybe I confused myself by using `N` as a block height when the PR at the time was already time based?

I think `BeginTime() { return MinBIP9WarningTime > nPowTargetTimespan ? MinBIP9WarningTime - nPowTargetTimespan : MinBIP9WarningTime; }` might make reasonable sense, with the idea being to check that there aren't any warning reported, then set `MinBIP9WarningTime` to a tim
...

🤔 mzumsande reviewed a pull request: "test: bugfix, synchronize indexes synchronously"
(https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1514862606)
> (An alternative, functionally equivalent to this, might be to just make the timeout infinite, see https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007, but I think this is fine as well.)

I think I'd prefer that option because then we wouldn't need to add a test-only arg to `BaseIndex::Start`, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and unit tests.

💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253333443)
Thanks for the explanation, makes sense. I got confused there somehow.

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1622067241)
@mzumsande This summary looks correct to me. Point (1) is covered by the functional test. You are also right that by default most nodes will not need the flag since there will be enough open slots or evictable peers.

I think the use case highlighted in the issue (as well as my own use case) is more about "personal" nodes or limited-resource nodes, where max connections are more limited but we still always want our light clients to be able to connect.

📝 achow101 reopened a pull request: "util/system: Close non-std fds when execing slave processes"
(https://github.com/bitcoin/bitcoin/pull/22417)
Currently, `RunCommandParseJSON` runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and starting bitcoind again will fail). It's also a potential security issue if a child process is intended to be sandboxed at some point. Not
...

👋 luke-jr's pull request is ready for review: "util/system: Close non-std fds when execing slave processes"
(https://github.com/bitcoin/bitcoin/pull/22417)