💬 laanwj commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3616479283)
Here's a patch to `manifest.scm` that makes it easier to test with arbitrary gcc commits: https://github.com/laanwj/bitcoin/commit/a323ddaf5ad88b3429e2ee079566efee4e0d8f7e
Allows:
```
GCC_COMMIT=953536524a198efa932ee2cccf5d29d4fc1d24f3 GCC_HASH=3b59e003321ac50462f626bba07826c2d4c203d4fd1500c64d2ea332e61eaab0 HOSTS="x86_64-w64-mingw32" contrib/guix/guix-build
```
- GCC_COMMIT is any arbitrary commit from the gcc repository (any will work as long as the patches apply).
- GCC_HASH is the he
...
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3616479283)
Here's a patch to `manifest.scm` that makes it easier to test with arbitrary gcc commits: https://github.com/laanwj/bitcoin/commit/a323ddaf5ad88b3429e2ee079566efee4e0d8f7e
Allows:
```
GCC_COMMIT=953536524a198efa932ee2cccf5d29d4fc1d24f3 GCC_HASH=3b59e003321ac50462f626bba07826c2d4c203d4fd1500c64d2ea332e61eaab0 HOSTS="x86_64-w64-mingw32" contrib/guix/guix-build
```
- GCC_COMMIT is any arbitrary commit from the gcc repository (any will work as long as the patches apply).
- GCC_HASH is the he
...
👍 hebasto approved a pull request: "[30.x] Backports & 30.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33997#pullrequestreview-3544326443)
re-ACK d59ebac7187dceb60f880ee3d2c5fab8368da176.
(https://github.com/bitcoin/bitcoin/pull/33997#pullrequestreview-3544326443)
re-ACK d59ebac7187dceb60f880ee3d2c5fab8368da176.
🤔 alexanderwiederin reviewed a pull request: "kernel: Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3544319469)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3544319469)
Concept ACK
💬 alexanderwiederin commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2592351617)
I noticed that `ProcessNewBlockHeaders` is called with `min_pow_checked` hard-coded to true.
Was this an intentional design choice? If callers are not supposed to to control this, it might be good to mention in the function docs that min-pow checks are never performed.
Otherwise, should we have min_pow checks be an option `btck_chainstate_manager_process_block_header`?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2592351617)
I noticed that `ProcessNewBlockHeaders` is called with `min_pow_checked` hard-coded to true.
Was this an intentional design choice? If callers are not supposed to to control this, it might be good to mention in the function docs that min-pow checks are never performed.
Otherwise, should we have min_pow checks be an option `btck_chainstate_manager_process_block_header`?
💬 sedited commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616489509)
It's not clear to me why this should happen through RPC if they are using the IPC interface already.
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616489509)
It's not clear to me why this should happen through RPC if they are using the IPC interface already.
💬 Sjors commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616505380)
@sedited there's different "they"s here:
1. The miner who proposes a block, they use IPC
2. The pool software that verifies the template and broadcasts a solution: the current SRI implementation doesn't use IPC
That said, I'm thinking about making an IPC method to fetch transactions more efficiently.
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616505380)
@sedited there's different "they"s here:
1. The miner who proposes a block, they use IPC
2. The pool software that verifies the template and broadcasts a solution: the current SRI implementation doesn't use IPC
That said, I'm thinking about making an IPC method to fetch transactions more efficiently.
💬 Fi3 commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616513802)
> It's not clear to me why this should happen through RPC if they are using the IPC interface already.
The [Template Provider](https://github.com/Sjors/sv2-tp/) provider is using the IPC interface but the Job Declarator Server is not, and it could need to get tx data for tx in jobs declared by the miner. Having a method to do that without asking them to the miner is very helpful. The Job Declarator Server could use the IPC interface, but I don't see why these should not be possible trough the
...
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616513802)
> It's not clear to me why this should happen through RPC if they are using the IPC interface already.
The [Template Provider](https://github.com/Sjors/sv2-tp/) provider is using the IPC interface but the Job Declarator Server is not, and it could need to get tx data for tx in jobs declared by the miner. Having a method to do that without asking them to the miner is very helpful. The Job Declarator Server could use the IPC interface, but I don't see why these should not be possible trough the
...
💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3616532437)
Slightly more minified:
```cpp
#include <filesystem>
#include <string>
#include <vector>
#include <cstring> // Unused, but removing this makes it determinstic
namespace fs {
using namespace std::filesystem;
// Dummy class. Using "path = std::filesystem::path" makes it deterministic.
class path : public std::filesystem::path
{
public:
using std::filesystem::path::path;
path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {}
};
}
bool
...
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3616532437)
Slightly more minified:
```cpp
#include <filesystem>
#include <string>
#include <vector>
#include <cstring> // Unused, but removing this makes it determinstic
namespace fs {
using namespace std::filesystem;
// Dummy class. Using "path = std::filesystem::path" makes it deterministic.
class path : public std::filesystem::path
{
public:
using std::filesystem::path::path;
path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {}
};
}
bool
...
💬 rkrux commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592395736)
It's documented that `LogPrintf` is deprecated, prefer to avoid its usage?
https://github.com/bitcoin/bitcoin/blob/0c9ab0f8f8c85719ff3aa4aefe3198cd2f8d63d1/src/logging.h#L372-L373
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592395736)
It's documented that `LogPrintf` is deprecated, prefer to avoid its usage?
https://github.com/bitcoin/bitcoin/blob/0c9ab0f8f8c85719ff3aa4aefe3198cd2f8d63d1/src/logging.h#L372-L373
💬 hebasto commented on issue "ci: failure in Windows native job":
(https://github.com/bitcoin/bitcoin/issues/34012#issuecomment-3616545810)
> I haven't looked here, but generally CI does not work well with re-running tasks, when the code is merged with current master, but the ci config is stale.
>
> As the error is zmq related, it could be due to [49c6728](https://github.com/bitcoin/bitcoin/commit/49c672853503e561cd1e7fed19a32f21ad345370), but again, I haven't checked this closely.
I concur with both assessments above.
The `.github/workflows/ci.yml` for the failed run looks outdated: https://github.com/bitcoin/bitcoin/actions/run
...
(https://github.com/bitcoin/bitcoin/issues/34012#issuecomment-3616545810)
> I haven't looked here, but generally CI does not work well with re-running tasks, when the code is merged with current master, but the ci config is stale.
>
> As the error is zmq related, it could be due to [49c6728](https://github.com/bitcoin/bitcoin/commit/49c672853503e561cd1e7fed19a32f21ad345370), but again, I haven't checked this closely.
I concur with both assessments above.
The `.github/workflows/ci.yml` for the failed run looks outdated: https://github.com/bitcoin/bitcoin/actions/run
...
💬 plebhash commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616550030)
currently SRI Job Declarator Server monitors the mempool via RPC and we have considered switching to ZMQ on this issue https://github.com/stratum-mining/sv2-apps/issues/26, which is about to be tackled soon
but I like @Sjors idea of leveraging IPC
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616550030)
currently SRI Job Declarator Server monitors the mempool via RPC and we have considered switching to ZMQ on this issue https://github.com/stratum-mining/sv2-apps/issues/26, which is about to be tackled soon
but I like @Sjors idea of leveraging IPC
👍 marcofleon approved a pull request: "[30.x] Backports & 30.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33997#pullrequestreview-3544399527)
ACK d59ebac7187dceb60f880ee3d2c5fab8368da176
(https://github.com/bitcoin/bitcoin/pull/33997#pullrequestreview-3544399527)
ACK d59ebac7187dceb60f880ee3d2c5fab8368da176
✅ hebasto closed a pull request: "qa: Remove no longer needed `feature_dirsymlinks.py`"
(https://github.com/bitcoin/bitcoin/pull/33924)
(https://github.com/bitcoin/bitcoin/pull/33924)
💬 sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2592425740)
This is on purpose from my part. I think it is confusing and not helpful to expose. If you look at where it is used, you'll see that all it does is invalidate a header that does not extend the pre-synced best work chain. The only case it is still used in our code is when processing unrequested blocks, but since we're processing headers here, I don't think that is relevant. I also don't think that this should be documented either. The current `min_pow_checked` handling in our validation code is c
...
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2592425740)
This is on purpose from my part. I think it is confusing and not helpful to expose. If you look at where it is used, you'll see that all it does is invalidate a header that does not extend the pre-synced best work chain. The only case it is still used in our code is when processing unrequested blocks, but since we're processing headers here, I don't think that is relevant. I also don't think that this should be documented either. The current `min_pow_checked` handling in our validation code is c
...
💬 Sjors commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616579875)
ZMQ will give you all new mempool transactions, whereas a new `getRawTransactions()` IPC method would only give you the ones you request. As does the RPC change suggested here.
That's probably enough if you keep a local cache of transactions you've seen before and only request unknown ones from `DeclareMiningJob`.
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616579875)
ZMQ will give you all new mempool transactions, whereas a new `getRawTransactions()` IPC method would only give you the ones you request. As does the RPC change suggested here.
That's probably enough if you keep a local cache of transactions you've seen before and only request unknown ones from `DeclareMiningJob`.
💬 fanquake commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592464223)
Yes. It should be removed soon: #29641.
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592464223)
Yes. It should be removed soon: #29641.
💬 l0rinc commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3616641991)
> It seems easier to just add Expected in its intended form in one go
Glad you have that alternative ready, closing in favor of https://github.com/bitcoin/bitcoin/pull/34006
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3616641991)
> It seems easier to just add Expected in its intended form in one go
Glad you have that alternative ready, closing in favor of https://github.com/bitcoin/bitcoin/pull/34006
✅ l0rinc closed a pull request: "util: generalize `util::Result` to support custom errors"
(https://github.com/bitcoin/bitcoin/pull/34005)
(https://github.com/bitcoin/bitcoin/pull/34005)
🤔 hodlinator reviewed a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3544455114)
Concept ACK fa1eee4f46fd089d6e19b124addee7c7679585f4
I prefer introducing `util::Expected` as a distinct type from `util::Result`, with a clear upgrade path to `std::expected` once we have C++23. (Maybe `util::Result` can later be implemented as a special case of `util::Expected`).
Recently suggested `std::variant` in another PR instead of using out-parameters (https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2584393520) but would strongly prefer `util::Expected`, had it existed.
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3544455114)
Concept ACK fa1eee4f46fd089d6e19b124addee7c7679585f4
I prefer introducing `util::Expected` as a distinct type from `util::Result`, with a clear upgrade path to `std::expected` once we have C++23. (Maybe `util::Result` can later be implemented as a special case of `util::Expected`).
Recently suggested `std::variant` in another PR instead of using out-parameters (https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2584393520) but would strongly prefer `util::Expected`, had it existed.
💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592454740)
This example shows that replacing `std::optional<ErrorType>` with `util::Expected<void, ErrorType>` is not totally risk-free as the old condition would still compile but be inverted. `err.value()` on the next line would have failed in this case though since it would return `void` which is an invalid argument type to the `JSONRPCError`-ctor.
It's tempting to add constraints to `value()` like so:
```C++
constexpr const T& value() const LIFETIMEBOUND
requires(!std::is_same_v<V, void>)
...
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592454740)
This example shows that replacing `std::optional<ErrorType>` with `util::Expected<void, ErrorType>` is not totally risk-free as the old condition would still compile but be inverted. `err.value()` on the next line would have failed in this case though since it would return `void` which is an invalid argument type to the `JSONRPCError`-ctor.
It's tempting to add constraints to `value()` like so:
```C++
constexpr const T& value() const LIFETIMEBOUND
requires(!std::is_same_v<V, void>)
...