💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265739284)
The approach to add a wtxid disguised as a txid to distinguish between legacy orphan processing and package relay seems a bit like a hack to me.
I'm not strictly against it, but I guess I don't completely understand yet why it's necessary - the information whether a peer supports package relay does not change, so why can't we just always use `GenTxid::Wtxid(wtxid)` and check again in `GetOrphanRequests()` and elsewhere whether we do package relay with the peer instead of checking whether it's
...

💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1264748895)
3494f96ebacf53cbd5fd09f6421d9dabec87361e:
should update this line too because fee is no longer included in it

💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265806240)
It's an unsolicited `NOTFOUND` of type `MSG_ANCPKGINFO`, would be good to distinguish between the inv type and the actual `ANCPKGINFO` message.

💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265811166)
`PkgInfoAllowed()` calling `ReceivedResponse()` as a side effect is surprising to me. Wouldn't it be better if the caller would also have to explicitly call `ForgetPkgInfo()` if they want to forget about the package? This is also not mentioned in its doc.

💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265856298)
We should also disable sending "sendpackages" for outgoing block-relay-only connections and probably also feelers, would suggest to just use `RejectIncomingTxs()`.
Also, I think this should be moved up and combined with the `m_enable_package_relay` check two lines above, because I see no reason to call `m_txpackagetracker->ReceivedVersion()` in these scenarios.

💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265838609)
I think that the splitting into commits is not ideal here: In ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we add logic to return the wtxid, expecting it to be requested by `MSG_ANCPKGINFO`. But the net_processing code to actually do this is only added in dd6fb139b8887f3400a65dc0d4c4ee54cf53d0bc, so that at ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we'd send out a `MSG_TX` getdata, thus re-requesting a TX we already know. These two things should go together into one commit.

💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265868269)
It makes it very difficult to read, imo, and lilkely introduces bugs going forward, if not now.

Note @mzumsande that this logic lives in the orphan relay PR IIRC

💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265869787)
Dropped the commit and instead added an `IsPrivacyNet()` helper for `GetLocal()` only, where it cleans things up. When we'll likely need to add `IsCJDNS()` to it later on, it will be simple to do and will keep `GetLocal()` easy to read.

💬 sipa commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1638845486)
Concept/approach ACK.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1265886880)
I'm looking into this, the [spec](https://www.jsonrpc.org/specification#notification) repeats no response to notifications but it doesn't indicate whether an HTTP OK is reasonable or whether the request should be completely ignored. I'm having some trouble getting libevent to completely cancel a request without sending anything back (and still letting bitcoind shutdown without hanging!) so maybe a 200 OK is best

💬 Empact commented on pull request "validation: use noexcept instead of deprecated throw()":
(https://github.com/bitcoin/bitcoin/pull/28090#issuecomment-1638877379)
utACK https://github.com/bitcoin/bitcoin/commit/047daad4f59942488163c6be8516a69291646294

💬 Empact commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#issuecomment-1638888808)
Rebased, converted assignments to initialization, and moved to `Assert` where possible. I could not apply `Assert(false)` in interpreter.cpp due to errors, but https://github.com/bitcoin/bitcoin/pull/26504 could be applied here once merged.

💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265915386)
right - I'll copy the comment so that we can discuss there - this one can be resolved.

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1265917707)

(moved over from [27742](https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265739284)):
The approach to add a wtxid disguised as a txid to distinguish between legacy orphan processing and package relay seems a bit like a hack to me.
I'm not strictly against it, but I guess I don't completely understand yet why it's necessary - the information whether a peer supports package relay does not change, so why can't we just always use `GenTxid::Wtxid(wtxid)` and check again in `GetOrphanRe
...

👍 jonatack approved a pull request: "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed""
(https://github.com/bitcoin/bitcoin/pull/28077#pullrequestreview-1533755139)
ACK ffa90fceae9b0c0ca2e720fae9e54dd3d094c988

Tested behavior before/after this pull using both the I2P Java router and with i2pd.

💬 achow101 commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1638966897)
ACK 4e5c933f6a40c07d1c68115f7979b89a5b2ba580

Verified that this was consistent with poly1305-donna.

🚀 achow101 merged a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993)

💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1638982355)
Thank you for the excellent review @stickies-v -- updated to take your feedback.

👋 sipa's pull request is ready for review: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008)

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1638997490)
Rebased on top of #28065, and marked ready for review.