🤔 mzumsande reviewed a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1531022386)
Some thoughts about logging:

After testing this on mainnet for a bit, I think it would be nice to be able to follow the fate of each individual orphan in `BCLog::TXPACKAGES`:
We currently have a tx-level based log entry for addition (“stored orphan..."), but nothing for removal, so having additional log entries in `MempoolAcceptedTx()` for succesful resolution and `MempoolRejectedTx()` for failure would be nice, maybe also a tx-level message for removal due to expiration and overflow.

If
...

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264170579)
Instead of using magic numbers, I've changed the test to use helper functions that check the major version.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264170660)
Done

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1636440917)
> [0ffa0f1](https://github.com/bitcoin/bitcoin/commit/0ffa0f1c370494b6b99a9cdf911b20ee655ac829) (and later) breaks at line line 96 when you run `wallet_backwards_compatibility.py --descriptors` without BDB:

Fixed

> > 0.16 creating new wallets rather than testing the target wallets
>
> Is there an assert you can add in [c31e0db](https://github.com/bitcoin/bitcoin/commit/c31e0db7f4263096d503f07707af9d0a733e50da) that would catch this directly? Right now a regression would be "caught" simp
...

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264177843)
Done.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264177911)
Done, thanks!

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264178108)
I think it's just a nice thing to do to ensure that all state is reset when the block index is cleared out and re-initialized; it might be surprising if state were leftover? But I agree that it doesn't seem strictly necessary. Added a comment to clarify.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264178256)
Updated the commit message.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264178307)
I'd say mostly for realism. My intuition for how this is designed is that if you don't reset the HAVE_DATA flags, then you'd immediately expect the background chain to process and validate all the blocks for which we HAVE_DATA and thus arrive at the original tip, as soon as you call ActivateBestChain().

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264179053)
We need this due to the CheckBlockIndex() invariant that you can't have a block index entry with nTx > 0 and without HAVE_DATA unless you've either pruned or used ASSUMED_VALID. Added a comment to clarify.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264179160)
Added a comment.

💬 furszy commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1636475094)
Little push to update the missing todo in the test. [Tiny diff](https://github.com/bitcoin/bitcoin/compare/47abfe1525cc2700699ba0605747f1ad36a34a1b..569748e526d6e33f0bcf8f19063e0bbdb241cfe4).

💬 achow101 commented on pull request "Show own outputs on PSBT signing window":
(https://github.com/bitcoin-core/gui/pull/740#issuecomment-1636535227)
ACK 4da243ba023f2987e97fc62886c6ebc70d6ee50a

💬 achow101 commented on pull request "Replace send-to-self with dual send+receive entries":
(https://github.com/bitcoin-core/gui/pull/119#discussion_r1264234276)
In f3fbe99fcf90daec79d49fd5d868102dc99feb23 "GUI: TransactionRecord: Refactor to turn send-to-self into send+receive pairs"

This for loop seems to be duplicated from the one above. I think this could be refactored so that we don't need to duplicate this work?

💬 jonatack commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1636603720)
Initial commit-by-commit build/review looks good, will do a full review.

💬 jamesob commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-1636628331)
I'll start another review early next week.

💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1264308888)
Okay “reconsiderable” makes sense to me, still have to review it’s correctly implemented.

> If it’s correct, I would still favor to exempt package transaction from min relay fee checks as it’s some awful interdependency for pre-signed lightning transaction :)

For this concern, in fact it’s already addressed by the latest state of bip 331 code and nversion=3 documentation, confusion is mine, I thought it was still present from previous package relay reviews.

💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1264309536)
I think this check is correct in what it aims to achieve, i.e not apply the two mempool min fee checks on ancestor package transaction, if it received as such from `AcceptAncestorPackage` / `ProcessNewPackage`, however this is unclear if this exemption apply for all BIP331 package, indifferently of their transactions versions, or only for nversion=3 ones.

For context, see https://github.com/lightningdevkit/rust-lightning/pull/2415#discussion_r1263189013

💬 vasild commented on 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#issuecomment-1636662332)
`e556bc6500...ffa90fceae`: try to fix windows compilation

💬 Realrobwoodx commented on issue "libsecp CI failure [no wallet, libbitcoinkernel] [focal]":
(https://github.com/bitcoin/bitcoin/issues/28079#issuecomment-1636688807)
Can someone tell me why this contract took all my Op tokens and how I can get them back without the feds?