💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1706054721)
done with the latest push.

💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2272057994)
> It may be worth logging something every time we ignore a peer that is on another chain.

[e977c69 ](https://github.com/bitcoin/bitcoin/commit/e977c698f97ac9bba30e4e3837f41721841c28c4)to [49d569c](https://github.com/bitcoin/bitcoin/commit/49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb):
Added a log message and slightly reworded a comment.

💬 LarryRuane commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-2272060737)
Force pushed rebase to fix conflict and also did some additional testing, I added several fake logging categories to ensure there can be more than 32 categories (the current limit), which is the main benefit of this pull. Also, I re-ran the benchmark tests (including with more than 32 categories), and this PR has no measurable performance impact.

@vasild and @ryanofsky, maybe you can take a look since you know this area, thanks.

💬 fjahr commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2272064338)
re-ACK 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb

Only trivial changes addressing comments, per diff: https://github.com/bitcoin/bitcoin/compare/e977c698f97ac9bba30e4e3837f41721841c28c4..49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb

💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706068877)
I think it's better to be precise and we could still shift it if that becomes absolutely necessary for a good reason. It seems better to have people prepare for it to be dropped in the next release unless there is good reason brought forward for shifting. This is better than the alternative that we are vague and then people claim last minute we can not do it in the next release already because we were not clear enough. I don't want to give an invite to the "Testnet3 is the real testnet" crowd :)
...

👍 ryanofsky approved a pull request: "test: [refactor] Use g_rand_ctx directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2222184242)
Code review ACK fa295215f6fa3b85b0387511920f75eeb3e12b58 but I think it would be nicer to add `FastRandomContext& m_rng{g_rand_ctx}` to `BasicTestingSetup` and have most test code use `m_rng` instead of `g_rand_ctx` directly.

I think this would be nicer mostly as a matter of style since most test code now doesn't reference global variables directly, while this PR is adding hundreds of references. But it could also help down the road, for example if we wanted to remove the global, or run test
...

💬 naiyoma commented on pull request "test: Use Decimal for fee calculations in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1706078135)
I was aiming for exact precision because of this comment https://github.com/bitcoin/bitcoin/pull/23225#issuecomment-938801112, but I get your point. Maybe if fee estimation changes to decimals internally in future, the tests can be modified accordingly.

💬 naiyoma commented on pull request "test: Use Decimal for fee calculations in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1706083059)
@maflcko IIUC reverting to the earlier version would mean that I also need to discard all the changes made to the functions `check_raw_estimates` and `check_smart_estimates`, right?

💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#discussion_r1706083475)
Worth crediting @1440000bytes too here, as they finalized my pull-req.

🤔 mzumsande reviewed a pull request: "lint: Find function calls in default arguments"
(https://github.com/bitcoin/bitcoin/pull/30553#pullrequestreview-2222204101)
Concept ACK, but someone who knows Rust better than I do will have to review it.

💬 ariard commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2272200826)
I think `mempoolfullrbf` could be left in core, if some users wishes to have first-seen of received transactions. E.g for lightning type `option_zeroconf` where double spend can only happen if (a) funding keys leak which is a bigger problem or (b) double-spend by the the zero-conf inbound conf seller, though here the paid invoices can be used as fraud proofs.

Ultimately, it’s a wider conversation to have mempool policy being more configurable in its own module as jtimon tried to implement yea
...

💬 ariard commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2272212885)
I would recommend to re-add in the backport release notes that excerpt from `24.x`:
https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-24.0.1.md

```
Contributors to this project strongly recommend that merchants and services
not accept unconfirmed transactions as final, and if they insist on doing so,
to take the appropriate steps to ensure they have some recourse or plan for
when their assumptions do not hold.
```

🤔 Shuhaib07 reviewed a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050#pullrequestreview-2222330434)
Approve

📝 fjahr opened a pull request: "assumeutxo: Drop block height from metadata"
(https://github.com/bitcoin/bitcoin/pull/30598)
Fixes https://github.com/bitcoin/bitcoin/issues/30514 which has more context.

This is an alternative approach to #30516 with much of the [code being suggested there](https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1689146902).

💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2272254485)
> How?

The height in the metadata clearly tells us which height the hash is expected at. Not having the same hash at that height clearly tells us that the node is on a different chain than was expected by the metadata. That is better than just saying "we don't know this hash for whatever reason".

> I also didn't find any discussion about the reason for the block height in https://github.com/bitcoin/bitcoin/pull/29612 on a quick glance (but didn't expand every comment).

It was suggested
...

💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2272260642)
The arguments here don't convince me yet that the height really has to be dropped necessarily but I also seem to be less passionate about this issue than @maflcko et al. So I opened the suggested approach of removing the block height in #30598 and I pinged other reviewers of the original metadata PR to weigh in shortly.

Ultimately what matters most to me is that we finally get this feature into the hands of users and I am fine to go along with either approach if it avoids further blocking of
...

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706154943)
In d60c6bba04f23f:

tiny nit: could cache the largest providers length instead of looping through the elements again here:

```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
--- a/src/script/descriptor.cpp (revision 8208454eae7484d34a162ffa7701157e56e3cb80)
+++ b/src/script/descriptor.cpp (date 1722981112377)
@@ -1818,6 +1818,7 @@
return {};
}
size_t script_size = 0;
+ size_t max_providers_length = 1; // if multipath was
...

🤔 furszy reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2222305361)
still reviewing, only nits so far.

> Although i agree it's more readable now it's unfortunate to ask to rewrite part of the parsing logic only for style purpose after it had 3 ACKs and underwent significant fuzzing.

I think it depends on the proposed changes. If readability is improved substantially, it helps maintenance and might also uncover logical issues that the fuzzer might not easily encounter. I don't think we should settle anything in stone because of a good fuzzing coverage. We
...

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706155751)
In https://github.com/bitcoin/bitcoin/commit/d60c6bba04f23f2956371b87dcc92c263ed5bb1e:

```suggestion
// For length 1 vectors, clone key providers until vector is the same length
```

👍 andrewtoth approved a pull request: "rpc, rest: Improve block rpc error handling, check header before attempting to read block data."
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2222373701)
utACK 02f6a42be3dfc57998304801c0d909bfa96745e4