💬 real-or-random commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2429613882)
@fanquake Can you elaborate on the motivation for this PR? The code in the `#else` should work for any gcc and clang, on any platform. While it uses an asm block, there's no actual asm instruction inside that could be platform-specific.
See also its platform-independent usage in Linux:
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux/string.h#L369-L376
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux
...
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2429613882)
@fanquake Can you elaborate on the motivation for this PR? The code in the `#else` should work for any gcc and clang, on any platform. While it uses an asm block, there's no actual asm instruction inside that could be platform-specific.
See also its platform-independent usage in Linux:
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux/string.h#L369-L376
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux
...
💬 fanquake commented on issue "Unable to compile for test coverage on Nixos 24.05":
(https://github.com/bitcoin/bitcoin/issues/31087#issuecomment-2429624992)
Note that this issue isn't Nix specifix, the coverage build type is somewhat broken (see also #31047). The steps to reproduce result in the same/similar failure on non-nix platforms.
(https://github.com/bitcoin/bitcoin/issues/31087#issuecomment-2429624992)
Note that this issue isn't Nix specifix, the coverage build type is somewhat broken (see also #31047). The steps to reproduce result in the same/similar failure on non-nix platforms.
🤔 theuni reviewed a pull request: "depends: add *FLAGS to gen_id"
(https://github.com/bitcoin/bitcoin/pull/31125#pullrequestreview-2385624483)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31125#pullrequestreview-2385624483)
Concept ACK
💬 theuni commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#discussion_r1810980328)
So why bother passing them? I don't see any need to rebuild the native packages when changing host flags?
(https://github.com/bitcoin/bitcoin/pull/31125#discussion_r1810980328)
So why bother passing them? I don't see any need to rebuild the native packages when changing host flags?
⚠️ TheBlueMatt opened an issue: "Expose `TestBlockValidity` over RPC"
(https://github.com/bitcoin/bitcoin/issues/31136)
### Please describe the feature you'd like to see added.
Not entirely sure it tests all types of block validity, but it looks like it nearly does? If it does, its generally quite useful to have a way to as Bitcoin Core if a block would be valid, ignoring its PoW, so having that be public would be nice.
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No resp
...
(https://github.com/bitcoin/bitcoin/issues/31136)
### Please describe the feature you'd like to see added.
Not entirely sure it tests all types of block validity, but it looks like it nearly does? If it does, its generally quite useful to have a way to as Bitcoin Core if a block would be valid, ignoring its PoW, so having that be public would be nice.
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No resp
...
👍 hebasto approved a pull request: "build: Fix kernel static lib component install"
(https://github.com/bitcoin/bitcoin/pull/31078#pullrequestreview-2385670659)
ACK 82e16e698321ea6fb69ce5a08b048347aab8c74e, tested on Ubuntu 23.10.
(https://github.com/bitcoin/bitcoin/pull/31078#pullrequestreview-2385670659)
ACK 82e16e698321ea6fb69ce5a08b048347aab8c74e, tested on Ubuntu 23.10.
💬 fanquake commented on issue "RPC breakage with v28.0":
(https://github.com/bitcoin/bitcoin/issues/31039#issuecomment-2429732718)
Going to leave this open a little longer to see if anything else comes up that might warrant a backport or other action.
(https://github.com/bitcoin/bitcoin/issues/31039#issuecomment-2429732718)
Going to leave this open a little longer to see if anything else comes up that might warrant a backport or other action.
💬 fanquake commented on issue "RPC breakage with v28.0":
(https://github.com/bitcoin/bitcoin/issues/31039#issuecomment-2429738588)
Possibly relevant: https://github.com/mempool/electrs/issues/102.
(https://github.com/bitcoin/bitcoin/issues/31039#issuecomment-2429738588)
Possibly relevant: https://github.com/mempool/electrs/issues/102.
💬 laanwj commented on issue "ci: Replace wine tests with real tests on Windows?":
(https://github.com/bitcoin/bitcoin/issues/31071#issuecomment-2429744574)
Concept ACK. WINE tests are better than nothing, but clearly running the tests on real windows is better. It is not our job to test WINE. They were introduced because there were no usable CI systems for windows at the time.
(https://github.com/bitcoin/bitcoin/issues/31071#issuecomment-2429744574)
Concept ACK. WINE tests are better than nothing, but clearly running the tests on real windows is better. It is not our job to test WINE. They were introduced because there were no usable CI systems for windows at the time.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811061628)
This only exists for sanity checking that we don't have more than one changeset at a time. We could just as easily replace this with a `bool`; I made it a pointer mostly for debugging purposes, if the `Assume()` were ever hit.
The idea behind the `unique_ptr` is that I wanted all the state in the ChangeSet to magically go away whenever the object goes out of scope, so that I didn't have to explicitly clean up state everywhere that a transaction might fail validation (which seems like a lot o
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811061628)
This only exists for sanity checking that we don't have more than one changeset at a time. We could just as easily replace this with a `bool`; I made it a pointer mostly for debugging purposes, if the `Assume()` were ever hit.
The idea behind the `unique_ptr` is that I wanted all the state in the ChangeSet to magically go away whenever the object goes out of scope, so that I didn't have to explicitly clean up state everywhere that a transaction might fail validation (which seems like a lot o
...
💬 mzumsande commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2429776658)
I do think that keeping `-port` around could be helpful, especially for users less knowledgeable about networking, who aren't sure which address to put in an equivalent `-bind` command.
I also like option 2, provided that the behavior will be well-documented, so that users specifying `-port` values for multiple nodes don't pick consecutive ones by accident (which would then again lead to collisions). Otherwise I don't see a downside.
I would also be ok with option 6, if I specify a port li
...
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2429776658)
I do think that keeping `-port` around could be helpful, especially for users less knowledgeable about networking, who aren't sure which address to put in an equivalent `-bind` command.
I also like option 2, provided that the behavior will be well-documented, so that users specifying `-port` values for multiple nodes don't pick consecutive ones by accident (which would then again lead to collisions). Otherwise I don't see a downside.
I would also be ok with option 6, if I specify a port li
...
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811072822)
I tried to make this code analogous to the old code, which repeated information for each transaction. I'm open to changing the logging but I am always a little afraid of breaking user's scripts or habits if people are used to grepping for a transaction ID and seeing more information? (Though I guess this PR already is breaking the logging for package RBF... so maybe not a big concern.)
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811072822)
I tried to make this code analogous to the old code, which repeated information for each transaction. I'm open to changing the logging but I am always a little afraid of breaking user's scripts or habits if people are used to grepping for a transaction ID and seeing more information? (Though I guess this PR already is breaking the logging for package RBF... so maybe not a big concern.)
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811074008)
Not sure if the tracing API is well-specified for package based replacement. I'm open to doing something different than what I propose here though, suggestions?
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811074008)
Not sure if the tracing API is well-specified for package based replacement. I'm open to doing something different than what I propose here though, suggestions?
💬 Christewart commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2429789160)
I'm in the camp of deprecating and then removing the `-port` setting. It seems we should should just get people to migrate to bind. Is `-port` strictly a subset of `-bind` functionality? If so it seems simplifying the config options would make sense.
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2429789160)
I'm in the camp of deprecating and then removing the `-port` setting. It seems we should should just get people to migrate to bind. Is `-port` strictly a subset of `-bind` functionality? If so it seems simplifying the config options would make sense.
💬 laanwj commented on pull request "rest: Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2429793735)
Concept NACK from me, sorry
i'd prefer to keep REST as read-only interface. As an unauthenticated interface, allowing any kind of posting exposes bitcoind to cross-site scripting attacks through web-browsers, for example.
There's also a scope-creep issue. If we expose more and more of the RPC interface on REST, we end up having to maintain and test two interfaces for everything. And they're not even so different.
> This would improve UX (no need to authenticate) and security (only a sma
...
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2429793735)
Concept NACK from me, sorry
i'd prefer to keep REST as read-only interface. As an unauthenticated interface, allowing any kind of posting exposes bitcoind to cross-site scripting attacks through web-browsers, for example.
There's also a scope-creep issue. If we expose more and more of the RPC interface on REST, we end up having to maintain and test two interfaces for everything. And they're not even so different.
> This would improve UX (no need to authenticate) and security (only a sma
...
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811083143)
Agreed, done.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811083143)
Agreed, done.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811083287)
Agreed, done.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811083287)
Agreed, done.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811084319)
Thanks that is much better; the only reason I had introduced `GetAggregateFeeRate()` was for access in this one place, so now that is gone.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1811084319)
Thanks that is much better; the only reason I had introduced `GetAggregateFeeRate()` was for access in this one place, so now that is gone.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2429804570)
Thanks @glozow and @ismaelsadeeq for the review; I've incorporated some of the comments. If you have suggestions for how the logging can be improved in the package RBF case (and how the USDT tracing messages can be improved, similarly) please let me know your thoughts.
Also, I wanted to flag that the locking is a bit of a mess in this branch. Initially, I tried to add lock annotations to the `CTxMemPoolChangeSet` functions to guarantee that the mempool itself was locked whenever any of thos
...
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2429804570)
Thanks @glozow and @ismaelsadeeq for the review; I've incorporated some of the comments. If you have suggestions for how the logging can be improved in the package RBF case (and how the USDT tracing messages can be improved, similarly) please let me know your thoughts.
Also, I wanted to flag that the locking is a bit of a mess in this branch. Initially, I tried to add lock annotations to the `CTxMemPoolChangeSet` functions to guarantee that the mempool itself was locked whenever any of thos
...
💬 laanwj commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2429812545)
i think the goal here was to make Windows code consistent between MSVC and mingw. MSVC is windows' official toolchain so it's leading for that. Using something else for Mingw could be considered a workaround.
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2429812545)
i think the goal here was to make Windows code consistent between MSVC and mingw. MSVC is windows' official toolchain so it's leading for that. Using something else for Mingw could be considered a workaround.