💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854105033)
Apologies l0rinc. I think we are probably just misunderstanding each other. All the checks you are suggesting are possible with the approach I'm suggesting, and it should only make them easier to implement and understand. And I might not understand what approach you prefer since your first comment https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850778259 seems to want new tests to be in the same commit as the bugfix, but the last one https://github.com/bitcoin/bitcoin/pull/31212#discu
...

💬 davidgumberg commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2494069961)
ACK https://github.com/bitcoin/bitcoin/commit/ee1128ead846698db5e5633f193883837f2fbc64

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1854157411)
Thank you!

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2494100649)
Several reindex related tests are broken, investigating...

> After https://github.com/bitcoin/bitcoin/pull/31283 it might make sense to drop the `waitTipChanged` method entirely too.

I think it's generically useful though. There are various types of applications that need to know when a new block is added to the chain.

👍 instagibbs approved a pull request: "rpc: Remove submitblock pre-checks"
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2455019300)
ACK 73db95c65c1d372822166045ca8b9f173d5fd883

Not an expert here but motivation makes a lot of sense, and good to remove such indirect code from past covert fixes

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2494237583)
Specifically `-reindex-chainstate` is broken, easy to reproduce with e.g. testnet4.

It stops after the second block:

```
2024-11-22T16:44:14.338986Z Setting NODE_NETWORK on non-prune mode
2024-11-22T16:44:14.339014Z Wait for genesis block to be processed
2024-11-22T16:44:14.339023Z initload thread start
2024-11-22T16:44:14.341109Z UpdateTip: new best=00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043 height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2024-05-03T23
...

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494293452)
@glozow I think I agree your branch at 4737df3f6512d2d9c7f8aa95e0635b9d03031402 is superior to this.

> Considering AcceptSubPackage is already smoothing out those joints, I like the idea of getting rid of ATMP in the future, just throwing things at ProcessNewPackage, and enumerating what we can handle at the start of AcceptPackage.

Yes, I would love to get rid of the separate paths over time.

> 've also implemented a removal of the is-child-with-unconfirmed-parents rule on top of it,
...

📝 yancyribbens opened a pull request: "Add coin-grinder example test"
(https://github.com/bitcoin/bitcoin/pull/31352)
In understanding the coin-grinder algorithm, I find it useful to run the algorithm with the parameters given in the example code. I thing it would be useful to add to the test-framework.

💬 glozow commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494305256)
thanks for looking!

> I'll have to do a bunch more thinking and clearly requires its own PR.
>
In case it wasn’t clear, I definitely meant that as a separate PR. I only
implemented it to sketch out what it’d look like as the next step.

>

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494334027)
Took @glozow first two commits with a couple additional sanity check lines.

💬 theStack commented on pull request "test: avoid internet traffic in rpc_net.py":
(https://github.com/bitcoin/bitcoin/pull/31343#discussion_r1854360802)
> What about using -connect=0 or (edit: this will not stop addnode) -proxy=127.0.0.1:1 for this particular test?

I agree that the -proxy approach (as done also in #31142) is much better and it seems to work as expected. Note that I added the parameter not only for the problematic sub-test, but for the whole functional test, in order to prevent also other sub-tests from connecting outside.

💬 theStack commented on pull request "test: avoid internet traffic in rpc_net.py":
(https://github.com/bitcoin/bitcoin/pull/31343#issuecomment-2494401283)
Thanks for reviewing @vasild! Force-pushed with your -proxy suggestion in https://github.com/bitcoin/bitcoin/pull/31343#discussion_r1853553795 and updated the PR description accordingly with test instructions.

💬 kevkevinpal commented on pull request "test: locking -testdatadir when not specified and then deleting lock and dir at end of test":
(https://github.com/bitcoin/bitcoin/pull/31328#issuecomment-2494403467)
> Code review [8963bc8](https://github.com/bitcoin/bitcoin/commit/8963bc860f6f55462851fbda28627b4483ba8240)
>
> Think this PR might be overly paranoid.
>
> Regardless of whether `G_TEST_GET_FULL_NAME` returns a value for the given test, is locking really necessary if directories are sufficiently unique (random) thanks to [faaaf59](https://github.com/bitcoin/bitcoin/commit/faaaf59f71ede057b2c1d369ef8db973c2f2dbc2) being included in the already merged #31317?
>
> ```c++
> const auto rand
...

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2494412531)
Tests passing, ready for re-review. I think we're close on this one.

💬 polespinasa commented on pull request "rpc: print P2WSH witScript in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r1854468502)
code rewritten based on this comment

📝 jonatack opened a pull request: "rpc, cli: add getbalances#total, and use it for -getinfo"
(https://github.com/bitcoin/bitcoin/pull/31353)
Add a "total" field in RPC getbalances to be able to easily see the total amount held in the wallet, and use that field for the wallet balances in CLI -getinfo.

Currently -getinfo only returns getbalances#mine.trusted for wallet balances. It would make sense to instead return the total balance. For instance, to see:

- watchonly balances
- coins returned to a wallet from an exchange or third party service that uses a fixed (reused) address, whether `avoid_reuse` is set or not, as it can b
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1854528651)
If #31346 lands first, `notifications().m_tip_block` could be an `Assert` and the comment removed, see https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2493956840

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854660293)
Long story short: I would find it suspicious if the test of a specific fix passes without the fix.
The cleanest way I found to make sure this isn't the case is to add the test commit before the fix commit and make sure CI fails if the test passes without the fix (see mentioned Spock [PendingFeature](https://github.com/spockframework/spock/blob/master/spock-core/src/main/java/spock/lang/PendingFeature.java#L13-L17)).
But since we don't have that here, I would prefer having the test in the same
...

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1854669075)
I'm a bit apprehensive on moving system-related code into the options. It just makes it a bit more complicated to patch out system-related code if we ever choose to ship support for the kernel library on bare metal. It is also why moved `system.h` into `common/` in the first place.

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1854672764)
Yeah, I thought of different approaches here too, happy to take an alternative here if somebody comes up with something. Otherwise we'll handle this once we get more queues, or a unified thread pool.