💬 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.

💬 achow101 commented on pull request "Descriptors: rule out unspendable miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1639007336)
ACK c7db88af71b3204171f33399aa4f33b40a4f7cd9

🚀 achow101 merged a pull request: "Descriptors: rule out unspendable miniscript descriptors"
(https://github.com/bitcoin/bitcoin/pull/27997)

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1639025450)
Anything further needed here?

🤔 jonatack reviewed a pull request: "Descriptors: rule out unspendable miniscript descriptors"
(https://github.com/bitcoin/bitcoin/pull/27997#pullrequestreview-1533865920)
Post-merge ACK

💬 jonatack commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1639117484)
Concept ACK

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1533749512)
This looks good, and thanks for all the clarifications. I'm just going over the commits another time in case I missed anything. Feel free to ignore all my suggestions, none are very important. Here's my progress so far:

- [X] fe86a7cd480b32463da900db764d2d11a2bea095 Explicitly track maximum block height stored in undo files (1/12)
- [X] 1cfc887d00c5d1d4281107e3b3ff4641c6c34631 Remove CChain dependency in node/blockstorage (2/12)
- [X] 471da5f6e74bac71aeffe2ebc5faff145a6cbcea Move block-arri
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266007380)
In commit "Move block-arrival information / preciousblock counters to ChainstateManager" (471da5f6e74bac71aeffe2ebc5faff145a6cbcea)

The "reload of the block index" comment now seems out of date. And actually I don't see why this `ClearBlockIndexCandidates` call is necessary here. After the InitializeChainstate call above, the active chainstate should be newly created and `setBlockIndexCandidates` should already be empty. So I think it would be good to drop this `ClearBlockIndexCandidates` cal
...