💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225416191)
Thanks. I'll try to get the Mining interface in good shape as well, which this can then be rebased on:
- #30409 (first commit)
- #30356
- to have `createNewBlock()` return an interface, see https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100
- add `waitFeesChanged()` (initially just uses `GetTransactionsUpdated()`, but once cluster mempool is in place it can do frequent (fast) block template construction to see if fees have actually gone up.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225416191)
Thanks. I'll try to get the Mining interface in good shape as well, which this can then be rebased on:
- #30409 (first commit)
- #30356
- to have `createNewBlock()` return an interface, see https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100
- add `waitFeesChanged()` (initially just uses `GetTransactionsUpdated()`, but once cluster mempool is in place it can do frequent (fast) block template construction to see if fees have actually gone up.
💬 paplorinc 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-2225417510)
@luke-jr, would this optimization be more welcome in https://github.com/bitcoin/libbase58 instead?
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2225417510)
@luke-jr, would this optimization be more welcome in https://github.com/bitcoin/libbase58 instead?
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675758398)
Thanks for the feedback, I really enjoyed removing the overloads based on the other comments! :)
Messed up the first force-push somehow, here is a comparison between what you reviewed and the current state:
https://github.com/bitcoin/bitcoin/compare/be23937392195c773811fdfc0d723783b2dace67..fcec222ae9386342ff1342e096255ca1ff74964d
> Also, could add a unit test that truncated input is parsed?
Do you mean that `base_blob::SetHex()` allows parsing "half bytes" like `"0xF"`, while `ParseHe
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675758398)
Thanks for the feedback, I really enjoyed removing the overloads based on the other comments! :)
Messed up the first force-push somehow, here is a comparison between what you reviewed and the current state:
https://github.com/bitcoin/bitcoin/compare/be23937392195c773811fdfc0d723783b2dace67..fcec222ae9386342ff1342e096255ca1ff74964d
> Also, could add a unit test that truncated input is parsed?
Do you mean that `base_blob::SetHex()` allows parsing "half bytes" like `"0xF"`, while `ParseHe
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675767961)
This is wrong and not a refactor. It crashes the node for me. Previously it did not crash.
```
node0 stderr terminate called after throwing an instance of 'std::out_of_range'
what(): basic_string_view::substr: __pos (which is 67) > __size (which is 66)
```
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675767961)
This is wrong and not a refactor. It crashes the node for me. Previously it did not crash.
```
node0 stderr terminate called after throwing an instance of 'std::out_of_range'
what(): basic_string_view::substr: __pos (which is 67) > __size (which is 66)
```
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675771306)
> Do you mean that `base_blob::SetHex()` allows parsing "half bytes" like `"0xF"`, while `ParseHex()` would fail on `"F"` and require `"0F"` (maybe `"F0"`)?
Yeah, it looks like this is already tested in `BOOST_AUTO_TEST_CASE(parse)`:
```cpp
{
std::string s_1{uint256::ONE.GetHex()};
BOOST_CHECK_EQUAL(uint256S("1\0").GetHex(), s_1);
BOOST_CHECK_EQUAL(uint256S(std::string{"1\0", 2}).GetHex(), s_1);
BOOST_CHECK_EQUAL(uint256S("0x1").GetHex(), s_1);
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675771306)
> Do you mean that `base_blob::SetHex()` allows parsing "half bytes" like `"0xF"`, while `ParseHex()` would fail on `"F"` and require `"0F"` (maybe `"F0"`)?
Yeah, it looks like this is already tested in `BOOST_AUTO_TEST_CASE(parse)`:
```cpp
{
std::string s_1{uint256::ONE.GetHex()};
BOOST_CHECK_EQUAL(uint256S("1\0").GetHex(), s_1);
BOOST_CHECK_EQUAL(uint256S(std::string{"1\0", 2}).GetHex(), s_1);
BOOST_CHECK_EQUAL(uint256S("0x1").GetHex(), s_1);
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675773282)
> Messed up the first force-push somehow, here is a comparison between what you reviewed and the current state:
> https://github.com/bitcoin/bitcoin/compare/be23937392195c773811fdfc0d723783b2dace67..fcec222ae9386342ff1342e096255ca1ff74964d
No worries. I do proper reviews locally anyway, as I don't trust GitHub, due to its history of data loss and data corruption.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675773282)
> Messed up the first force-push somehow, here is a comparison between what you reviewed and the current state:
> https://github.com/bitcoin/bitcoin/compare/be23937392195c773811fdfc0d723783b2dace67..fcec222ae9386342ff1342e096255ca1ff74964d
No worries. I do proper reviews locally anyway, as I don't trust GitHub, due to its history of data loss and data corruption.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675775175)
This is fixed in C++23. Also, most C++17 compilers would already issue a compile warning for this.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675775175)
This is fixed in C++23. Also, most C++17 compilers would already issue a compile warning for this.
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225488531)
Updated 8e9367371b47d7a88136a80d0cb091e1f32d2a4e -> 3c0a13c9a56a11d795b243cedb3b00a71a81042c ([`pr/mine.1`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.1) -> [`pr/mine.2`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.1..pr/mine.2)) filling in missing serialization for CBlockTemplate and BlockValidationState types, also fixing link errors for test programs that were causing CI tasks to fail.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2225488531)
Updated 8e9367371b47d7a88136a80d0cb091e1f32d2a4e -> 3c0a13c9a56a11d795b243cedb3b00a71a81042c ([`pr/mine.1`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.1) -> [`pr/mine.2`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.1..pr/mine.2)) filling in missing serialization for CBlockTemplate and BlockValidationState types, also fixing link errors for test programs that were causing CI tasks to fail.
💬 pythcoiner commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2225488932)
ACK 00337ef
I've written some [tests](https://github.com/bitcoin/bitcoin/commit/f6cdf9bec82533e849f6156f5593dc9ceb688306) and tested import descriptors produced by liana wallet
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2225488932)
ACK 00337ef
I've written some [tests](https://github.com/bitcoin/bitcoin/commit/f6cdf9bec82533e849f6156f5593dc9ceb688306) and tested import descriptors produced by liana wallet
💬 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.
...