💬 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):
)

#
...

💬 Sjors commented on pull request "interface: Expose load utxo snapshot functionality":
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2837856588)
@D33r-Gee I'm a bit confused what the plan is here. Do you intend to implement this feature in the current GUI, or only in the QML project?

In the plan is to implement it here, which would be great, I would suggest opening two pull requests:

1. The entire feature, including the interface change commit, here - but marked as draft
2. The interface change commit, against the Bitcoin Core repo. Conceptually this in an interface ON the node FOR the GUI, so it belong in that repo. From there yo
...

💬 Sjors commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-2837862858)
@mzumsande does that diagram look right to you?

💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837886136)
> Anything that can appear in your mempool via `OP_RETURN` can already appear as an inscription, at 1/4th the cost for the attacker.

Incorrect. Because 'inscriptions' are an exploit, they have to break up that data into ~500 byte chunks partitioned by PUSH opcodes. This means the largest contiguous piece of raw data to contend with is ~500 bytes.

With unlimited sized OP_RETURN, we now have to deal with ~100KB contiguous pieces of data... 200x more. Far more dangerous from an external anal
...

💬 Turtlecute33 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837901444)
Hey, I’d like to ask why my comment above (https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2801335235) was marked as a duplicate. I believe it includes some technical points that weren’t cited earlier, for example, that allowing more OP_RETURN usage doesn’t necessarily mean spammers will stop using witness-discounted Taproot outputs.
(cc @pinheadmz)