💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996244623)
Why is this not re-used for `ProcessNewBlock`?

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995939302)
Nit: `s/this a/this is a/`

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995921074)
Nit: Are we still doing `-present` now? I thought I saw some discussion about that recently, but can't find it anymore.

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996249639)
I'm surprised this is not just taking the `m_chainstates` size. Why is that?

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996263354)
I'm a bit confused when it is appropriate to use `::cs_main` and when `chainman.GetMutex()`. Is there a rule to that?

📝 hodlinator opened a pull request: "net: Only try downgrade v2->v1 transport if online"
(https://github.com/bitcoin/bitcoin/pull/32073)
We might have just set `fDisconnect` in the preceding loop because of being offline.

Attempting to reconnect using v1 transport just because `fNetworkActive` was set to `false` at the "right" stage in the v2 handshake does not make sense.

Issue [discovered](https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1930908304) by davidgumberg.

💬 hodlinator commented on pull request "net: Only try downgrade v2->v1 transport if online":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2725820358)
#### How I tested

Run node on mainnet:
```shell
bitcoind -debug=net -daemonwait
```
Tail log for relevant info:
```shell
tail -f ~/.bitcoin/debug.log | grep -E "SetNetworkActive|Network not active|retrying"
```
In another console, spam:
```shell
build/bin/bitcoin-cli setnetworkactive false && sleep 0.5 && build/bin/bitcoin-cli setnetworkactive true
```

Expected grep results:
```
2025-03-14T21:12:17Z SetNetworkActive: false
2025-03-14T21:12:17Z [net] Network not active, discon
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725832595)
Re https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725437236 and https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725441239

Thank you for your suggestions!

> Would it be possible to use these C headers to automatically build a rust crate of constants from a C header API? Or would that be overkill..

Looking at the constants in the linked file they all seem to be policy-related, which is out of scope for now. I don't think we'll add a header for that in the near fut
...

💬 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.