💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909139264)
Interesting, I do agree there is a spaghetti/complexity factor to `expected_ret_code`, so might revisit this.

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2580812651)
Updated based of ryanofsky's [review](https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2532868324).

Hopefully latest PR summary is slightly clearer.

Replaced "test" -> "qa" since PR is about functional tests and not unit tests, should conform better with *CONTRIBUTING.md*.

💬 ryanofsky commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2580824337)
> Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they're built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.

I think I don't understand what you are saying. If clients can connect to the node and call waitNext() to wait for block templates, then the node can "decide when it wants to push a new block, enabling predi
...

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909184706)
I like the suggestion, but after implementing it still does not seem entirely ideal. I just made the entire `DBParams` field an optional, so it's not easily possible to set only the cache size, but not the path. It is also annoying that the current behaviour with the path is that it is dependent on the data dir path, not the blocks dir path.

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909193453)
Good thinking ahead! Yeah, my idea was to use something along those lines too. Maybe we could start annotating some of the functions here to make it easier for the developer to see which exceptions it throws? Though my understanding is that such annotations have been deliberately left out of the code so far (https://stackoverflow.com/questions/1055387/throw-keyword-in-functions-signature).

On the other hand it would be nice if we could agree on more coherent error handling for constructors, b
...

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2580871894)
Updated 384c73d4fcda4af7c2b4bfb945a248cb93dba47d -> b776e40554c8a315d832f3996d14ff2e3ae7b8cb ([blockmanDB_6](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_6) -> [blockmanDB_7](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_6..blockmanDB_7))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909124834), ran clang-format over commits.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909199419)
I did this to address https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903268301

Let me know if (`LARGE_TXS_COUNT` - 1) + `NORMAL_TXS_COUNT` is preferable.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909201069)
I've taken your suggestion @Sjors.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909201883)
I've taken this with a little modification.

👋 brunoerg's pull request is ready for review: "descriptor: check whitespace in pubkeys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603)

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2580884607)
Ready for review!

Thanks, @furszy for the comment. I've changed the approched and described it on description.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208594)
> This changes the meaning of `getmininginfo` fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.

> We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.

I've reverted and included it but clarify the `getmininginfo` help text.

> I agree we should not change the `weight`, `currentblockweight` and `sigops` fields in `getblocktemplate` and `getmi
...

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208803)
Done.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2580905232)
Thanks for reviewing @pinheadmz @luke-jr @Sjors

I've recently foreced pushed from 2fb833e69627a8f9b1c4b14b60d1df9903adf210 to 7ab1344eb13228998bcead7328d5cc0a905971d4 [diff](https://github.com/bitcoin/bitcoin/compare/2fb833e69627a8f9b1c4b14b60d1df9903adf210..7ab1344eb13228998bcead7328d5cc0a905971d4)

Changes were:

1. Reverting https://github.com/bitcoin/bitcoin/commit/76a95522b322c1a8ab1594ca8bf5ac99ab379c46 but improving the help text.
2. Use policy `DEFAULT_MAX_BLOCK_WEIGHT` instead
...

💬 theuni commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580925856)
> > And doing that all over would be a recipe for never-ending churn
>
> Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?

Not change for change's sake, no. Please consider this from the POV of reviewers and maintainers. A constant stream of changes like that is exhausting. It would distract from larger improvements.

I think @maflcko's comment above is a good d
...

📝 mzumsande opened a pull request: "wallet: fix rescanning inconsistency"
(https://github.com/bitcoin/bitcoin/pull/31629)
If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting `m_last_processed_block`, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474.
Fix this by not rescanning blocks beyond `m_last_processed_block` - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.

This means that
...

💬 mzumsande commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2580943009)
See #31629, I took your version, just added some doc changes.

💬 l0rinc commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580961020)
This PR was a follow-up from the comments on Marco's change.
The main changes are Russ's template simplification and my suggestion to const the writes to obviate which we're modifying.

💬 sipa commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2580988597)
ACK b7f3b6fd96ca2d5d1f9538affef682f778bd07e8

In the log message `[bench] FlushStateToDisk: write coins cache to disk (598202 coins, 86527KiB) started`, the number of coins and the size is the total cache size now, including non-dirty entries. I guess it should be changed to just account for the dirty ones, as well as in the "Flushing large UTXO set to disk" warning message.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2581015366)
> it should be changed to just account for the dirty ones

That would be a follow-up from #28233.