💬 fjahr commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1996327219)
Added it back now

💬 fjahr commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1996327284)
done

💬 fjahr commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1996327542)
I have reworked that part a bit, I hope it's clearer now.

💬 fjahr commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#issuecomment-2725842683)
Addressed the comments now since there were a few that made sense to fix.

> New to this test, please correct me know if am wrong, after checking the history of the file, it seems that it has primarily been used to test deprecated RPCs both with and without the -deprecatedrpc flag. This change proposes to focus exclusively on testing the behavior of deprecated RPCs without the flag, as other tests already cover the cases where the -deprecatedrpc flag is used.

Yes, that's correct!

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996324766)
> nit - you can simplify this line slightly by using the default arguments instead of passing them explicitly.

No, the way this method is written, it is possible to pass in a custom CoinSelectionParams object and it will be passed through to MakeCoin. If I adopted the change you propose, the MakeCoin call here would be limited to the default parameters.

🤔 murchandamus reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2686906009)
> Concept ACK to refactor the tests. `TestBnBSuccess` and `TestBnBFail` are much better and easier to read. Thanks!

I’m glad to read that.

> > non-representative and effectuate counterintuitive behavior such as feerate = 0 or cost_of_change = 0
>
> I'm still trying to sus out what the tests are doing now that's better than before.

For example, our waste calculation used to assume that `cost_of_change` being zero indicated that the transaction is changeless and any excess should be d
...

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996329365)
I added to the description that the change weights are taken from the P2WPKH output type.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725912225)
Rebased 29513955891e40e78466f2c666dfa13e9c1b2914 -> 21f6a3de77a9eedcca5d47f694d540d42b3ddbcc ([kernelApi_27](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_27) -> [kernelApi_28](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_28), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_27..kernelApi_28))

* Fixed conflict with #31649

💬 fjahr commented on pull request "test: Check datadir cleanup after assumeutxo was successful":
(https://github.com/bitcoin/bitcoin/pull/32033#discussion_r1996379732)
That's a good point, we might be getting the name wrong later and the test becomes meaningless. I addressed this and took that opportunity to refactor it a bit and remove the duplication.

💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2725980816)
Thanks again for the hint @ryanofsky, I went through all changes again and [reworked the batching](https://github.com/bitcoin/bitcoin/compare/177a3bff143ae41a536b4d2b5a44a255b768bf32..5a1c2bd341c9586b090b6b40c20edb011cb616eb) - it's so much better this way and it includes both your idea and Cory's.

Additionally I fixed a few things after rebase:
* There was a leftover `SaveBlock` from the previous refactor;
* Commits and docs still contained `build/src/benc/bench_bitcoin` - changed them to
...

💬 aref136677 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8#r153756606)
1111111111111111111114oLvT2

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996418239)
Ah I see, even though that wouldn't cause any test failures, that would limit the functions behavior in the future. My mistake.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2725992821)
> Which commit is this that you are linking to?

This is master from only a few days ago when I submitted the review feedback.

👍 darosior approved a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2687058377)
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996429149)
Good idea to simplify this by reducing the size of the set. Two sevens here with a reduced target amount serves the same purpose as four sevens.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996431719)
This is really minor nit, although I find the `/*selection_target=*` comments to be more distracting than helpful personally.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996437543)
It would be nice if BnB returned the `total_tries`, like coin_grinder. I recently added that to the rust implementation since I think it's useful for regression testing. If BnB was ever refactored or had changes it would come in handy imo.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2726030060)
> We would expect at low feerates the input set with more inputs to be preferred, but instead the selection with the smaller change budget is preferred.

I'm familiar with change_budget as it exists in coin-grinder. Beyond that, I'm not sure how that ties in to BnB and SRD.

> Please see the commit "test: Move BnB feerate sensitivity tests" and the commit message of the commit.

I reviewed the commits, and it _appears_ that the test coverage is equivalent. It might be helpful to document
...

💬 pinheadmz commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2726033041)
> My understanding from the low-level networking discussion at CoreDev was that this wouldn't build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!

Sure, by coredev I had already written most of this implementation (based on sockman) but the performance was bad, and that was part of the motivation behind the deep-dive talk. However, by the end of the week I had reviewed that code in pe
...

🤔 Prabhat1308 reviewed a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2687270174)
Concept ACK [`3301d2c`](https://github.com/bitcoin/bitcoin/pull/32010/commits/3301d2cbe8c3b76c97285d75fa59637cb6952d0b)
I could not recreate a race condition but I have
- Tested that the commented files dont trigger a startup error.
- Ran all the tests to check that all pass .