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

📝 vasild converted_to_draft a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326)
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.

Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the
...

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2837679823)
Converted to draft, waiting for:

* https://github.com/bitcoin/bitcoin/pull/32338 to make it
* then change this PR to use naming as in https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2063650801

Further improvements away from this PR:
* make `m_nodes_mutex` non-recursive, https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2835048806
* assess all the checks that compare just the address or address+port and remove redundant ones and make the calls consistent regardless of
...

💬 shahsb commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837703405)
It's great to see constructive discussions taking place here!

I don't see any valid reason to take away configuration settings from users.

Just because certain config options don't directly influence what miners include in blocks doesn't mean users lose control over what appears in their mempools.

Therefore, my take is: Concept NACK.

👍 Sjors approved a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2802349810)
ACK 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72

Thanks for adding the tests.

Nits:
- maybe rename the first commit title to: "Drop OP_RETURN standardness limits for datacarrier size and output count"-
- squash 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72 into it
- see inline comment for test improvement

---

> Considering the floodgates of absolutely horror this PR is going to bring upon Bitcoin node runners

Anything that can appear in your mempool via `OP_RETURN` can already appear as
...

💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2065694091)
In 8fd09d622d161eb554f080a52735832119cb35e5 "Add more OP_RETURN mempool acceptance functional tests"

This this is now a bit redundant, so might as well expand it to clarify something:

```diff
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index a4dfda8443..2ac4905a57 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -338,9 +338,10 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
)

#
...