💬 Fi3 commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616470606)
ACK for dmnd or other pools that will implement sv2 and want to do job declaration and being able to relay the blocks founded by the miners, having fast way to retrieve tx data from the node using a wtxid would be very useful.
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616470606)
ACK for dmnd or other pools that will implement sv2 and want to do job declaration and being able to relay the blocks founded by the miners, having fast way to retrieve tx data from the node using a wtxid would be very useful.
💬 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.