💬 maflcko commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1802848071)
Thanks, clarified the name, because it wasn't in a namespace itself (only the global one).

🤔 sdaftuar reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2368652846)
Read through the code (except for the fuzz test, which I plan to go back and review). Concept ACK.

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800758237)
Do we need this `assert()`? Can we just do an`Assume()` instead?

My general philosophy is that we should avoid calling `assert()` in codepaths that run on data received from the network, unless it really would be better for (potentially) the ENTIRE network to go down rather than just continue, in the event that the assert() can be remotely triggered.

I see that we might crash further down if we somehow there is a nullptr here, so my suggestion for error handling would be to `Assume()` eve
...

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800930436)
I don't think it makes sense to gate this on `!bypass_limits` -- doing so only has an effect for transactions that spend a non-dust output that are coming from the same block as a transaction that creates a dust output, which seems like both (a) a very small effect that (b) we don't obviously want to do...

Specifically, it seems strange that we would only accept a transaction that was reorged out of a block that has a dust output if it has exactly 1 such dust output and exactly 0 fee, yet we
...

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802693519)
nit: s/spend/spent/ ?

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800908033)
I would love to merge the codepaths for the package and non-package case, so that we have no concerns over whether all the right checks have been run. What if we did something like this:

- Add all passed-in transactions to a map: Txid -> CTransactionRef. This will allow us to look up transactions either locally (from a package) or in the mempool.

- For every transaction passed in:
* For every input in the transaction:
- Look up the input in the local map. If not found, look up in
...

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802744124)
Can we assert that this transaction is different from the one already in the mempool?

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802833358)
Did you mean `const CTransactionRef& tx_ref` here?

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802727338)
Just want to make sure I understand this comment, does "any size" refer to the value in the output?

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802745380)
Might be nice to assert that this transaction has a higher feerate than the one in the mempool, just to be sure that this would be a successful RBF if not for the ephemeral dust issue.

💬 dergoegge commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2416431400)
I opened #31093, implementing the suggestion from https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400559220.

💬 stickies-v commented on issue "Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/issues/31017#issuecomment-2416432329)
> Using RPC requires an authenticated connection.

Could you please elaborate on why RPC or authentication is not a feasible option for your use case?

🤔 stickies-v reviewed a pull request: "rest: Support transaction broadcast in REST interface"
(https://github.com/bitcoin/bitcoin/pull/31065#pullrequestreview-2372023562)
I'd prefer not extending the REST interface if we don't have to. This functionality is already well supported in the RPC, so this seems like purely a convenience endpoint?

I've asked for clarification in https://github.com/bitcoin/bitcoin/issues/31017, but until there's a good reason to add this I'm Concept NACK.

💬 glozow commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802861498)
This is what testmempoolaccept does

🤔 instagibbs reviewed a pull request: "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs"
(https://github.com/bitcoin/bitcoin/pull/24128#pullrequestreview-2372024041)
concept ACK, appears to adhere to the BIP on first glance

will do deeper review if tests are applied

💬 maflcko commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1802863026)
Either should be fine, because it just ends up in the debug log (not really for end-users). What about:

```cpp
throw std::runtime_error(strprintf("File version (%d) too high to be read.", nVersionRequired));
```

if `up-version` is too vague?

💬 maflcko commented on issue "Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/issues/31017#issuecomment-2416454459)
> > Using RPC requires an authenticated connection.
>
> Could you please elaborate on why RPC or authentication is not a feasible option for your use case?

I am not familiar with electrum protocol, but the issue description says:

> An indexer like the ones for the electrum protocol should work without having access to the RPC interface

💬 maflcko commented on pull request "rest: Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2416456307)
I don't think the REST interface will be removed any time soon, or ever, so if users want this, it seems fine to add. But no strong opinion.

🚀 fanquake merged a pull request: "build: Bump minimum supported macOS to 13.0"
(https://github.com/bitcoin/bitcoin/pull/31048)

🤔 mzumsande reviewed a pull request: "Halt processing of unrequested transactions v2"
(https://github.com/bitcoin/bitcoin/pull/30572#pullrequestreview-2372049564)
> See the latest state of the code (`23551ef`), which I think have been pushed before your comment publication. There is a `ExpectedTx()` method checking in the `TxRequestTracker` if the transaction has been requested either by wtxid or txid, orphan or not. Currently, Bitcoin Core is doing parent orphan fetching by txid: https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L4682
> so there should no parent orphan received as an unsolicited transaction, and not found as `REQUEST
...