💬 TheCharlatan commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2840157913)
Concept ACK
> An alternative approach could be to make Cap'n Proto optional and have -DENABLE_IPC default to ON only if its present. Though I vaguely recall that with CMake we wanted to move away from enabling config options depending on whether a library is found.
Yeah, this is intentionally not done anymore since the cmake migration. I think the current approach is fine.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2840157913)
Concept ACK
> An alternative approach could be to make Cap'n Proto optional and have -DENABLE_IPC default to ON only if its present. Though I vaguely recall that with CMake we wanted to move away from enabling config options depending on whether a library is found.
Yeah, this is intentionally not done anymore since the cmake migration. I think the current approach is fine.
💬 fjahr commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2840158373)
Concept ACK to remove the limits on OP_RETURN (both size and count). I don't see a need to remove the config options at this point and would support keeping them in at this point, with a deprecation message. Though that is not a blocker for me.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2840158373)
Concept ACK to remove the limits on OP_RETURN (both size and count). I don't see a need to remove the config options at this point and would support keeping them in at this point, with a deprecation message. Though that is not a blocker for me.
📝 w0xlt opened a pull request: "Make `cs_db` mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32382)
Make `cs_db` mutex non-recursive (related to https://github.com/bitcoin/bitcoin/issues/19303).
(https://github.com/bitcoin/bitcoin/pull/32382)
Make `cs_db` mutex non-recursive (related to https://github.com/bitcoin/bitcoin/issues/19303).
💬 l0rinc commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2840165283)
Would it be possible to summarize the pros and cons raised across the internets in the PR description - if for no other reason, to avoid repeated comments on the same topic? Maybe it would help bring the two sides together on this admittedly contentious issue, where both have valid concerns and good arguments.
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2840165283)
Would it be possible to summarize the pros and cons raised across the internets in the PR description - if for no other reason, to avoid repeated comments on the same topic? Maybe it would help bring the two sides together on this admittedly contentious issue, where both have valid concerns and good arguments.
💬 jarolrod commented on pull request "interface: Expose load utxo snapshot functionality":
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2840173805)
@D33r-Gee no, your interface changes are part of what gets merged into the qml repo. It is part of the changeset to implement your assumeutxo work -> in the qml repo all in the qml repo. Then the process of packaging it up to upstream is, primarily, outside of your concern. I am just mentioning this to show why this shouldn't be open here.
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2840173805)
@D33r-Gee no, your interface changes are part of what gets merged into the qml repo. It is part of the changeset to implement your assumeutxo work -> in the qml repo all in the qml repo. Then the process of packaging it up to upstream is, primarily, outside of your concern. I am just mentioning this to show why this shouldn't be open here.
📝 w0xlt converted_to_draft a pull request: "Make `cs_db` mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32382)
Make `cs_db` mutex non-recursive (related to https://github.com/bitcoin/bitcoin/issues/19303).
(https://github.com/bitcoin/bitcoin/pull/32382)
Make `cs_db` mutex non-recursive (related to https://github.com/bitcoin/bitcoin/issues/19303).
✅ w0xlt closed a pull request: "Make `cs_db` mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32382)
(https://github.com/bitcoin/bitcoin/pull/32382)
💬 nsvrn commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2840175804)
"allow" when no one is asking for this and has no positive effect or other basis, on that reason Concept NACK
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2840175804)
"allow" when no one is asking for this and has no positive effect or other basis, on that reason Concept NACK
💬 maflcko commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2067406415)
it would be better to file this separately and explain why it isn't needed (or rather since when)
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2067406415)
it would be better to file this separately and explain why it isn't needed (or rather since when)
💬 achow101 commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-2840221541)
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-2840221541)
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
💬 TheCharlatan commented on pull request "refactor: validation: mark CheckBlockIndex as const":
(https://github.com/bitcoin/bitcoin/pull/32364#issuecomment-2840221780)
Concept ACK
I wrote this simpler approach here https://github.com/TheCharlatan/bitcoin/commit/fe13fa082e3d6befa6d5332428584570ead608ab after seeing you mentioning it last week. I think making sure that all block indexes are declared const in the method might be beneficial though. Maybe instead of adding a separate `GetAll` method just make it const and then declare the chainstate pointers as const in the for loop?
(https://github.com/bitcoin/bitcoin/pull/32364#issuecomment-2840221780)
Concept ACK
I wrote this simpler approach here https://github.com/TheCharlatan/bitcoin/commit/fe13fa082e3d6befa6d5332428584570ead608ab after seeing you mentioning it last week. I think making sure that all block indexes are declared const in the method might be beneficial though. Maybe instead of adding a separate `GetAll` method just make it const and then declare the chainstate pointers as const in the for loop?
💬 BTCMcBoatface commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2840235544)
ACK, but actually NACK. What if you leave the `datacarrier` and `datacarriersize` config options but remove the enforcement code: filter-enjoyers don’t get mad, code is cleaner, limits remain ineffective.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2840235544)
ACK, but actually NACK. What if you leave the `datacarrier` and `datacarriersize` config options but remove the enforcement code: filter-enjoyers don’t get mad, code is cleaner, limits remain ineffective.
🚀 achow101 merged a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039)
(https://github.com/bitcoin/bitcoin/pull/29039)
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2067435003)
We don't "need" the check - but what we want to guarantee is that whenever `cs_main` is not held, the block index must be in a consistent state (i.e. `CheckBlockIndex()` would pass) - because at this point, other threads can change the block index / call CBI, leading to issues such as #16444.
This spot is pretty critical, because even though `InvalidateBlock` will clean up in the end, it releases `cs_main` for stability reasons multiple times (so that the node doesn't become unresponsive for to
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2067435003)
We don't "need" the check - but what we want to guarantee is that whenever `cs_main` is not held, the block index must be in a consistent state (i.e. `CheckBlockIndex()` would pass) - because at this point, other threads can change the block index / call CBI, leading to issues such as #16444.
This spot is pretty critical, because even though `InvalidateBlock` will clean up in the end, it releases `cs_main` for stability reasons multiple times (so that the node doesn't become unresponsive for to
...
💬 achow101 commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#issuecomment-2840281637)
ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
(https://github.com/bitcoin/bitcoin/pull/32338#issuecomment-2840281637)
ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
💬 BrazyDevelopment commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2840289771)
I have significant concerns about removing the `datacarrier` and `datacarriersize` limits without thoroughly addressing the potential for exacerbating existing attack vectors.
Bitcoin's mempool and network already face challenges from known attack vectors like flood and loot attacks, replacement cycling, and irreversible fee attacks, which exploit transaction propagation and fee mechanics to burden nodes or disrupt confirmation reliability.
By removing OP_RETURN limits, we risk introducin
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2840289771)
I have significant concerns about removing the `datacarrier` and `datacarriersize` limits without thoroughly addressing the potential for exacerbating existing attack vectors.
Bitcoin's mempool and network already face challenges from known attack vectors like flood and loot attacks, replacement cycling, and irreversible fee attacks, which exploit transaction propagation and fee mechanics to burden nodes or disrupt confirmation reliability.
By removing OP_RETURN limits, we risk introducin
...
💬 achow101 commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-2840297221)
ACK abe43dfadd6325f80975a76aea57a549c3162191
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-2840297221)
ACK abe43dfadd6325f80975a76aea57a549c3162191
🚀 achow101 merged a pull request: "net: remove unnecessary check from AlreadyConnectedToAddress()"
(https://github.com/bitcoin/bitcoin/pull/32338)
(https://github.com/bitcoin/bitcoin/pull/32338)
💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2067466850)
> Is there any value in documenting the version used twice with the additional maintenance overhead and risk of it being stale? The depends package definition is already linked, so anyone can get the real version with just one more click/open. If they want to find out the pull request/commit it will be just another step (git blame).
>
> Could consider removing it?
I absolutely agree, I can add a commit to remove the "Version used" column from all tables if everyone is fine with this.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2067466850)
> Is there any value in documenting the version used twice with the additional maintenance overhead and risk of it being stale? The depends package definition is already linked, so anyone can get the real version with just one more click/open. If they want to find out the pull request/commit it will be just another step (git blame).
>
> Could consider removing it?
I absolutely agree, I can add a commit to remove the "Version used" column from all tables if everyone is fine with this.
✅ achow101 closed an issue: "Log which peer sent us a header (first)"
(https://github.com/bitcoin/bitcoin/issues/27744)
(https://github.com/bitcoin/bitcoin/issues/27744)