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

💬 Realrobwoodx 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-1636690172)
I want out of this contract I don't even know what it is or how it linked to me but someone needs to send back my funds in the amount of 35.588 OP tokens or, ultimately I will involve the authorities. This isn't a joke I consider this theft/fraud return to me ASAP 

💬 hebasto commented on pull request "bugfix: Make `CCheckQueue` RAII-styled (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1636693701)
Rebased 6e079015707188ac15405662ce396dd837da569a -> 2235765f7fd946d3b08fd9e109584967407e46d1 ([pr26762.13](https://github.com/hebasto/bitcoin/commits/pr26762.13) -> [pr26762.14](https://github.com/hebasto/bitcoin/commits/pr26762.14)) due to the conflict with #28048.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1264388335)
> But I'm not able to see another reason why we should add the txid to the `inventory_known_filter`, so why not delete this whole block?

That seems correct to me.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1264391225)
It's okay to discard it though -- you just might as well not have called the function in the first place if you don't care about the return value? (And given it's inline, presumably the compiler will treat it as if it wasn't called anyway).

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1636756516)
> Why not just use C++ then? (I'm not a Rust expert, maybe there are good reasons)

I tried that, but vanilla C++ doesn't have a nice interface for processes. There's only libc, or third party libs. So it seemed easier to just use rust with the added bonus that the written code closely resembles the look and feel of python. If you write something elegant in C++, I am happy to push it here, tough.