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

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266149966)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (eb8ce95a2551fe521d00a5901ea773514395255b)

Is this comment in the commit message still accurate?

> Note that this introduces the new invariant that we can only have an assumeutxo
> snapshot where the snapshotted blockhash is in our block index. As a followup,
> unit tests that mock creating a snapshot ought to be updated to reflect this.

The only way I can see that it relates to code changes in this commit
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266147442)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (eb8ce95a2551fe521d00a5901ea773514395255b)

Note: test here is changed to add the real block 1, instead of a random block, so the block will not be ignored by the new `snapshot_base->GetAncestor(pindex->nHeight) == pindex` check in TryAddBlockIndexCandidate

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266156727)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (eb8ce95a2551fe521d00a5901ea773514395255b)

Seems like it might be useful to check `BOOST_CHECK_EQUAL(exp_tip, exp_tip2);` now to verify `ActivateBestChain` actually did arrive at the expected CBlockIndex*, not just the right block block hash.