💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225503069)
I'll work on dropping the spawn functionality from this PR next, and just having the `bitcoin-mine` program print an error when it can't connect to the node socket, instead of starting the node itself. Sjors suggested this in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225041208:
> If it helps getting IPC stuff merged quicker, it's fine by me if the miner can't spin up a node process (see e.g. [Sjors@b6dfa5f](https://github.com/Sjors/bitcoin/commit/b6dfa5f186e8a4813db21b2547de
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225503069)
I'll work on dropping the spawn functionality from this PR next, and just having the `bitcoin-mine` program print an error when it can't connect to the node socket, instead of starting the node itself. Sjors suggested this in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225041208:
> If it helps getting IPC stuff merged quicker, it's fine by me if the miner can't spin up a node process (see e.g. [Sjors@b6dfa5f](https://github.com/Sjors/bitcoin/commit/b6dfa5f186e8a4813db21b2547de
...
⚠️ hebasto opened an issue: "depends: `libevent` build fails when `C{C,XX}` are provided"
(https://github.com/bitcoin/bitcoin/issues/30439)
```
$ make -C depends libevent HOST=x86_64-apple-darwin CC=clang CXX=clang++
make: Entering directory '/home/hebasto/git/bitcoin/depends'
Configuring libevent...
-- The C compiler identification is Clang 16.0.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /usr/bin/clang-16
-- Check for working C compiler: /usr/bin/clang-16 - broken
CMake Error at /usr/share/cmake-3.27/Modules/CMakeTestCCompiler.cmake:67 (message):
The C c
...
(https://github.com/bitcoin/bitcoin/issues/30439)
```
$ make -C depends libevent HOST=x86_64-apple-darwin CC=clang CXX=clang++
make: Entering directory '/home/hebasto/git/bitcoin/depends'
Configuring libevent...
-- The C compiler identification is Clang 16.0.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /usr/bin/clang-16
-- Check for working C compiler: /usr/bin/clang-16 - broken
CMake Error at /usr/share/cmake-3.27/Modules/CMakeTestCCompiler.cmake:67 (message):
The C c
...
💬 fanquake commented on issue "depends: `libevent` build fails when `C{C,XX}` are provided":
(https://github.com/bitcoin/bitcoin/issues/30439#issuecomment-2225515985)
Overriding those variables when cross-compiling for macOS isn't currently supported.
(https://github.com/bitcoin/bitcoin/issues/30439#issuecomment-2225515985)
Overriding those variables when cross-compiling for macOS isn't currently supported.
💬 fanquake commented on issue "depends: `libevent` build fails when `C{C,XX}` are provided":
(https://github.com/bitcoin/bitcoin/issues/30439#issuecomment-2225521100)
IIRC we also settled on the minimum clang for macOS being 18, so the Clang you're using (16), is likely too old.
(https://github.com/bitcoin/bitcoin/issues/30439#issuecomment-2225521100)
IIRC we also settled on the minimum clang for macOS being 18, so the Clang you're using (16), is likely too old.
💬 fanquake commented on issue "depends: `libevent` build fails when `C{C,XX}` are provided":
(https://github.com/bitcoin/bitcoin/issues/30439#issuecomment-2225523404)
Note that this is also not libevent specific. It's just that it happens to be the first thing compiled in depends.
(https://github.com/bitcoin/bitcoin/issues/30439#issuecomment-2225523404)
Note that this is also not libevent specific. It's just that it happens to be the first thing compiled in depends.
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675824921)
Hm.. may have managed to figure out why it would not fail before. Old code:
```C++
std::string strTxid = uriParts[i].substr(0, uriParts[i].find('-'));
std::string strOutput = uriParts[i].substr(uriParts[i].find('-')+1);
```
If `.find('-')` returned `npos` and `npos = size_type(-1)`, then `+1` would make it `0`, so it would not throw in `substr()`. This would happen for an empty `uriParts[i]`, or non-empty not containing `-` (in this latter case it might have failed parsing `strOutput` if en
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675824921)
Hm.. may have managed to figure out why it would not fail before. Old code:
```C++
std::string strTxid = uriParts[i].substr(0, uriParts[i].find('-'));
std::string strOutput = uriParts[i].substr(uriParts[i].find('-')+1);
```
If `.find('-')` returned `npos` and `npos = size_type(-1)`, then `+1` would make it `0`, so it would not throw in `substr()`. This would happen for an empty `uriParts[i]`, or non-empty not containing `-` (in this latter case it might have failed parsing `strOutput` if en
...
💬 theuni commented on pull request "depends: bump boost to 1.85.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1675904484)
Blah. I started working on it, but there's no precedent for a `cmake -DHEADER_ONLY` in the boost libs and their CMake files are auto-generated. So I assumed there'd be resistance to a patch like this.
But now that we have a reason, sure, I'll patch them and PR the patches and see how it goes.
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1675904484)
Blah. I started working on it, but there's no precedent for a `cmake -DHEADER_ONLY` in the boost libs and their CMake files are auto-generated. So I assumed there'd be resistance to a patch like this.
But now that we have a reason, sure, I'll patch them and PR the patches and see how it goes.
💬 theuni commented on pull request "depends: bump boost to 1.85.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2225598972)
Pushed a workaround hack in the meantime.
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2225598972)
Pushed a workaround hack in the meantime.
✅ hebasto closed an issue: "depends: `libevent` build fails when `C{C,XX}` are provided"
(https://github.com/bitcoin/bitcoin/issues/30439)
(https://github.com/bitcoin/bitcoin/issues/30439)
💬 hebasto commented on pull request "depends: bump boost to 1.85.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1675935057)
Do we want to avoid it when building depends on macOS?
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1675935057)
Do we want to avoid it when building depends on macOS?
📝 Sjors opened a pull request: "Have createNewBlock() return a BlockTemplate interface "
(https://github.com/bitcoin/bitcoin/pull/30440)
Suggested in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a `NewTemplate` message (which doesn't include transactions) as quickly as possible.
Builds on #30356 to avoid rebase hell, but does not require it.
...
(https://github.com/bitcoin/bitcoin/pull/30440)
Suggested in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a `NewTemplate` message (which doesn't include transactions) as quickly as possible.
Builds on #30356 to avoid rebase hell, but does not require it.
...
💬 fanquake commented on pull request "depends: bump boost to 1.85.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1675940381)
None of the compiled libraries are used, so if this is scoped to Boost, why would it make any difference?
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1675940381)
None of the compiled libraries are used, so if this is scoped to Boost, why would it make any difference?
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225616156)
> filling in missing serialization for `CBlockTemplate`
You won't need that if you rebase on #30440, though maybe it's better to wait until that's merged to avoid too much code churn.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225616156)
> filling in missing serialization for `CBlockTemplate`
You won't need that if you rebase on #30440, though maybe it's better to wait until that's merged to avoid too much code churn.
🤔 jonatack reviewed a pull request: "cli: improve error message on multiwallet and add validation to cli-side commands"
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-2175003345)
Thanks for updating! I think 6ccf7cb946bd3772b5fba92246be7e2281f59d09 is good to go.
WIP review on commit 3d63fc976d616436d64335b15a918ffba1883b9a but I initially find it confusing. The diffs below are between your commit and testing some modifications to it.
For instance, the tests that pass a wallet CLI option to a non-wallet CLI command (-generate) seem to be mixing issues under test...
```diff
- # Single or Multi wallet modes don't matter here as -generate command valid
...
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-2175003345)
Thanks for updating! I think 6ccf7cb946bd3772b5fba92246be7e2281f59d09 is good to go.
WIP review on commit 3d63fc976d616436d64335b15a918ffba1883b9a but I initially find it confusing. The diffs below are between your commit and testing some modifications to it.
For instance, the tests that pass a wallet CLI option to a non-wallet CLI command (-generate) seem to be mixing issues under test...
```diff
- # Single or Multi wallet modes don't matter here as -generate command valid
...
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225687227)
> > filling in missing serialization for `CBlockTemplate`
>
> You won't need that if you include #30440, though maybe it's better to wait until that's merged to avoid too much code churn.
Yeah definitely some of that should not be needed. Some parts will probably be needed though. For example, the initial version of this PR did not support deserializing `CTransaction` or `CBlock` objects (because `CTransaction` does not have an `Unserialize` method, and also requires a version parameter t
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225687227)
> > filling in missing serialization for `CBlockTemplate`
>
> You won't need that if you include #30440, though maybe it's better to wait until that's merged to avoid too much code churn.
Yeah definitely some of that should not be needed. Some parts will probably be needed though. For example, the initial version of this PR did not support deserializing `CTransaction` or `CBlock` objects (because `CTransaction` does not have an `Unserialize` method, and also requires a version parameter t
...
📝 Sjors opened a pull request: "[WIP] Add getCoinbaseMerklePath() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30441)
Builds on #30440.
This adds `getCoinbaseMerklePath()` to the Mining interface.
Unlike the entire `Mining` interface so far, this is _not_ a refactor. It adds new functionality.
The Stratum v2 [NewTemplate](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client) message is a short message intended to give miners something to do as quickly as possible. It does not include the serialized block transactions. In lieu of that i
...
(https://github.com/bitcoin/bitcoin/pull/30441)
Builds on #30440.
This adds `getCoinbaseMerklePath()` to the Mining interface.
Unlike the entire `Mining` interface so far, this is _not_ a refactor. It adds new functionality.
The Stratum v2 [NewTemplate](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client) message is a short message intended to give miners something to do as quickly as possible. It does not include the serialized block transactions. In lieu of that i
...
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225699350)
I'll certainly need `CBlock`. It's probably good to support `CTransaction` as well, as it might allow for more efficiently fetching only missing transactions in a template update, but that would be a future optimisation.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225699350)
I'll certainly need `CBlock`. It's probably good to support `CTransaction` as well, as it might allow for more efficiently fetching only missing transactions in a template update, but that would be a future optimisation.
💬 marcofleon commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2225719837)
> Does it improve if you call `SeedRandomForTest` before every execution of every fuzz input?
Yes, calling `SeedRandomForTest(SeedRand::ZEROS)` at the beginning of every iteration improves stability from 28% to 70%.
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2225719837)
> Does it improve if you call `SeedRandomForTest` before every execution of every fuzz input?
Yes, calling `SeedRandomForTest(SeedRand::ZEROS)` at the beginning of every iteration improves stability from 28% to 70%.
💬 luke-jr commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2225722849)
>would this optimization be more welcome in https://github.com/bitcoin/libbase58 instead?
The algorithm used in libbase58 is different, so not sure this even applies. Kinda doubt it would be worth the time to port/review either, unless a library consumer cares about performance or the other criteria explained already here.
If you're just looking for things to do, extending libbase58 to Bech32 might be worth doing. See https://github.com/bitcoin/libbase58/issues/6
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2225722849)
>would this optimization be more welcome in https://github.com/bitcoin/libbase58 instead?
The algorithm used in libbase58 is different, so not sure this even applies. Kinda doubt it would be worth the time to port/review either, unless a library consumer cares about performance or the other criteria explained already here.
If you're just looking for things to do, extending libbase58 to Bech32 might be worth doing. See https://github.com/bitcoin/libbase58/issues/6
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2225766504)
> Yes, calling SeedRandomForTest(SeedRand::ZEROS) at the beginning of every iteration improves stability from 28% to 70%.
Cool, I'll address it.
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2225766504)
> Yes, calling SeedRandomForTest(SeedRand::ZEROS) at the beginning of every iteration improves stability from 28% to 70%.
Cool, I'll address it.