💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2065255362)
I had forgotten why I did it that way, but when I tried that, testing the first commit results in warnings that the helper functions are unused.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2065270057)
Taken, thanks

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2065274288)
I did that on purpose to illustrate that the function now needs the custom weight for the new test

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2837277525)
> nit: Perhaps the commit descriptions ("BnB rate sensitivity tests", "simple BnB failure tests", and "BnB iteration exhaustion test", for example) could become functions for clarity (not necessarily new test cases).

I’m not sure I understand what you mean. Do you mean that I introduce helper functions like "TestBnBSuccess" for the new tests instead of making them their own test cases?

📝 LEGENDZENT opened a pull request: "4 million lost bitcoin"
(https://github.com/bitcoin/bitcoin/pull/32370)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ achow101 closed a pull request: "4 million lost bitcoin"
(https://github.com/bitcoin/bitcoin/pull/32370)

📝 achow101 locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/32370)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837312049)
> Luckily, blocks and mempool data are stored obfuscated on disk by XOR-ing with a random per-node key. See #28052, #28207.

This, admittedly, partly mitigates one objection of mine (out of dozens, to be clear). I recall this was discussed _many_ years ago in #bitcoin-dev, but I didn't know it actually finally martialized. Thanks.

However, unless I'm missing something, it seems regular upgrade paths don't have a mechanism to make any use of this obfuscation. Only new nodes seem to benefit.
...

💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837355636)
@wizkid057 I need to remind people that Bitcoin Core is not the only implementation out there. It is already quite easy to get arbitrarily sized OP_Returns mined, and even Bitcoin Core's standardness rules already allow long byte sequences to get mined and thus included in block*.dat files.

Anyway, I just checked, and Google Chromium v135 at least appears to store cached data on your hard drive verbatim (plus a file header). If even they aren't concerned about unobfuscated random cached data
...

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2837376769)
> nit: Some commits can be squashed to avoid warnings like "unused function".

@w0xlt: I ran each commit, and I did not receive this warning. Could you let me know which commit resulted in that warning for you?



> As discussed during Bitcoin Core Review Club, at least one test with 0 fee rate should probably be added to avoid test coverage regression.

@monlovesmango: I added another commit to run the simple tests at various feerates, including the feerates 0 sat/kvB, 1 sat/kvB, and 1,
...

💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837460876)
> @wizkid057 I need to remind people that Bitcoin Core is not the only implementation out there. It is already quite easy to get arbitrarily sized OP_Returns mined, and even Bitcoin Core's standardness rules already allow long byte sequences to get mined and thus included in block*.dat files.

By "long byte sequences", I don't know of any way to get more than few hundred bytes in a sequence without a miner's involvement.

I'm talking about entire real world files without breaks.

It's alre
...

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2837465545)
Rebased due to strange CI failures

⚠️ maflcko opened an issue: "Issue in wallet_backwards_compatibility.py self.test_v22_inactivehdchain_path() assert addr is not None"
(https://github.com/bitcoin/bitcoin/issues/32371)
Example: https://cirrus-ci.com/task/6425957709381632?logs=ci#L2161

💬 achow101 commented on issue "Issue in wallet_backwards_compatibility.py self.test_v22_inactivehdchain_path() assert addr is not None":
(https://github.com/bitcoin/bitcoin/issues/32371#issuecomment-2837514390)
#32369

💬 maflcko commented on pull request "test: Use the correct node for doubled keypath test":
(https://github.com/bitcoin/bitcoin/pull/32369#issuecomment-2837521146)
lgtm ACK 32d55e28af69ef09094ba921289bf4d1ad79905a

I saw the silent merge conflict in the two pulls and wanted to mention it as soon as one gets merged, but it looks like both got merged at the same time.

✅ maflcko closed an issue: "Issue in wallet_backwards_compatibility.py self.test_v22_inactivehdchain_path() assert addr is not None"
(https://github.com/bitcoin/bitcoin/issues/32371)

💬 maflcko commented on issue "Issue in wallet_backwards_compatibility.py self.test_v22_inactivehdchain_path() assert addr is not None":
(https://github.com/bitcoin/bitcoin/issues/32371#issuecomment-2837522436)
Thanks. Let's just continue discussion there.

💬 maflcko commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2837605050)
There are also some typos, according to https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2828844270, if you want to fix them if you have to re-touch for other reasons anyway.

👍 rkrux approved a pull request: "test: Use the correct node for doubled keypath test"
(https://github.com/bitcoin/bitcoin/pull/32369#pullrequestreview-2802202219)
ACK 32d55e28af69ef09094ba921289bf4d1ad79905a

Ah, I did message on IRC yesterday to get #29124 merged because I realised there'd be conflicts when I was reviewing #32350. Looks like the latter got merged first.

Idk why the bot didn't add the "Needs rebase" tag on #29124 when the other got merged, I can see a gap of around an hour between the two merges. I'm sensing the CI runs tests after merges that takes some time for the bot to update open PRs with the required tags.

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2065614037)
True, and `ConnectNode()` is only called from `OpenNetworkConnection()`. Looks like another room for improvement.

While this may seem minor, the amount of developers' time spent on grasping it is not. In the past I have stared at these calls disproportionally long. Would be nice if this is spared to future developers. I will go after that separately from this PR.