💬 willcl-ark commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1467385670)
Not specifically related to the issue at hand: the new podman desktop app is pretty smart, it can show you containers running via docker and podman (and kubernetes clusters) all in the same UI: https://podman-desktop.io/
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1467385670)
Not specifically related to the issue at hand: the new podman desktop app is pretty smart, it can show you containers running via docker and podman (and kubernetes clusters) all in the same UI: https://podman-desktop.io/
💬 glozow commented on pull request "doc: clarify `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467389150)
I agree with @stickies-v that listing all the callers of a function within code documentation is not maintainable. This should not be something that a developer relies on and is what a grepper / search bar is for. If it's confusing that not all RPCs are listed, I'd be in favor of just deleting the names of the RPCs that use this function, as it doesn't matter to the comment.
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467389150)
I agree with @stickies-v that listing all the callers of a function within code documentation is not maintainable. This should not be something that a developer relies on and is what a grepper / search bar is for. If it's confusing that not all RPCs are listed, I'd be in favor of just deleting the names of the RPCs that use this function, as it doesn't matter to the comment.
🤔 maflcko reviewed a pull request: "test: Check object hashes in wait_for_getheaders"
(https://github.com/bitcoin/bitcoin/pull/29318#pullrequestreview-1845337916)
In Bitcoin Core, the stop hash will always be zero, so not sure if it is worth it to require passing it every time?
It may be better to instead pass the top hash and check that, if provided?
(https://github.com/bitcoin/bitcoin/pull/29318#pullrequestreview-1845337916)
In Bitcoin Core, the stop hash will always be zero, so not sure if it is worth it to require passing it every time?
It may be better to instead pass the top hash and check that, if provided?
💬 maflcko commented on pull request "test: Check object hashes in wait_for_getheaders":
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397387)
I think the block hash can be returned from `announce_random_block` and then checked here against the first item in the block locator hashes?
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397387)
I think the block hash can be returned from `announce_random_block` and then checked here against the first item in the block locator hashes?
💬 maflcko commented on pull request "test: Check object hashes in wait_for_getheaders":
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397574)
same?
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397574)
same?
💬 maflcko commented on pull request "test: Check object hashes in wait_for_getheaders":
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397689)
same?
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397689)
same?
💬 maflcko commented on pull request "test: Check object hashes in wait_for_getheaders":
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467398533)
same?
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467398533)
same?
💬 ismaelsadeeq commented on pull request "doc: update `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467426255)
Deleted the comment, and updated the PR description.
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467426255)
Deleted the comment, and updated the PR description.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1911778393)
Please take high-level and LN usage discussions elsewhere.
- Lightning usage, including switching {commitment, HTLC-X} transactions to using {v3, ephemeral anchors} is being discussed here: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/24
- General v3 discussion can go here: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340/32
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1911778393)
Please take high-level and LN usage discussions elsewhere.
- Lightning usage, including switching {commitment, HTLC-X} transactions to using {v3, ephemeral anchors} is being discussed here: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/24
- General v3 discussion can go here: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340/32
💬 fanquake commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911784362)
> So enabling that compiler warning may not be possible without changing a lot more code.
Yea. See here for example output compiling master with GCC 13.2.0 + -Wsign-conversion: https://gist.github.com/fanquake/8e7a49dc1968afd07d89e822ffb4adac.
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911784362)
> So enabling that compiler warning may not be possible without changing a lot more code.
Yea. See here for example output compiling master with GCC 13.2.0 + -Wsign-conversion: https://gist.github.com/fanquake/8e7a49dc1968afd07d89e822ffb4adac.
💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1467461085)
Yeah I think passing those in would be a bit unhygienic. `ApplyV3Rules` would be using the contents beforehand to decide whether a child would be replaced, and then potentially modifying it in a way that looks like the child would replace it.
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1467461085)
Yeah I think passing those in would be a bit unhygienic. `ApplyV3Rules` would be using the contents beforehand to decide whether a child would be replaced, and then potentially modifying it in a way that looks like the child would replace it.
💬 tromp commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911816807)
Nobody is defending the exploit. Rather, some of us recognize that this exploit is inherent in Bitcoin's design, whose script language provides plenty of room for embedding arbitrary data. What transactions miners put in blocks is driven purely by profit motives and the fact is that inscriptions are very profitable. Declaring them "bad" doesn't change that. As long as they follow consensus rules, they are "good" to the miners.
The intentions behind this proposal are good. What I object to is
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911816807)
Nobody is defending the exploit. Rather, some of us recognize that this exploit is inherent in Bitcoin's design, whose script language provides plenty of room for embedding arbitrary data. What transactions miners put in blocks is driven purely by profit motives and the fact is that inscriptions are very profitable. Declaring them "bad" doesn't change that. As long as they follow consensus rules, they are "good" to the miners.
The intentions behind this proposal are good. What I object to is
...
💬 maflcko commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1911903716)
> nit: it seems the return value from SerializeFileDB is never used to actually shut down the node. In the case of failing to open file, I think LogError is appropriate because that probably should lead to a shutdown. In the case of "Failed to flush file", I think a LogWarning could be more appropriate.
>
> I think it's best to leave the PR as is to make progress, bikeshedding over warning vs error categories doesn't have a huge amount of benefit here. Just leaving this comment for visibility.
...
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1911903716)
> nit: it seems the return value from SerializeFileDB is never used to actually shut down the node. In the case of failing to open file, I think LogError is appropriate because that probably should lead to a shutdown. In the case of "Failed to flush file", I think a LogWarning could be more appropriate.
>
> I think it's best to leave the PR as is to make progress, bikeshedding over warning vs error categories doesn't have a huge amount of benefit here. Just leaving this comment for visibility.
...
💬 fanquake commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1467547030)
> My feeling is just that setting SECP_CFLAGS is not the cleanest choice and more likely than my other suggestions to cause breaks in the future.
We can accept that possibility for now, and at the next subtree update (if/when things break), we'll take another look.
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1467547030)
> My feeling is just that setting SECP_CFLAGS is not the cleanest choice and more likely than my other suggestions to cause breaks in the future.
We can accept that possibility for now, and at the next subtree update (if/when things break), we'll take another look.
💬 fanquake commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1467547077)
These lines have never been super consistent, but are meant to be a rough overview of the (interesting) flags being used for compilation. I think you're correct in that printing secp flags (again) here would be overkill. For any other flags, I don't think they are very meaningful for most builders. I think it's also unlikely we'll significantly change anything here too much further, given that this is going to be completely redone with CMake.
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1467547077)
These lines have never been super consistent, but are meant to be a rough overview of the (interesting) flags being used for compilation. I think you're correct in that printing secp flags (again) here would be overkill. For any other flags, I don't think they are very meaningful for most builders. I think it's also unlikely we'll significantly change anything here too much further, given that this is going to be completely redone with CMake.
✅ fanquake closed an issue: "libsecp256k1 not instrumented when building with sanitizers"
(https://github.com/bitcoin/bitcoin/issues/27990)
(https://github.com/bitcoin/bitcoin/issues/27990)
🚀 fanquake merged a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875)
(https://github.com/bitcoin/bitcoin/pull/28875)
✅ dergoegge closed a pull request: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043)
(https://github.com/bitcoin/bitcoin/pull/28043)
💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1911941270)
It's been more than 6 months since I opened this without substantial review, so closing. Let me know if anyone is actually interested in reviewing and I'll reopen.
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1911941270)
It's been more than 6 months since I opened this without substantial review, so closing. Let me know if anyone is actually interested in reviewing and I'll reopen.
💬 torkelrogstad commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#discussion_r1467562973)
Ah, thanks. I'm very new to C++, so I didn't realize I could even use things that weren't imported. Didn't realize I should also include a header statement, since it compiled just fine! Added the include.
(https://github.com/bitcoin/bitcoin/pull/29175#discussion_r1467562973)
Ah, thanks. I'm very new to C++, so I didn't realize I could even use things that weren't imported. Didn't realize I should also include a header statement, since it compiled just fine! Added the include.