💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1675726351)
@glozow
> probably `FeeFrac` would be better? `CFeeRate` loses precision and can be inferred from the fee and size
We will be using the package size to calculate the percentile package and select its fee rate as an estimate to return to the user.
We then have to convert the fee and size to fee rate either by `CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes).GetFeePerK` or introduce and use a similar `GetFeeRate` method for the `FeeFrac` struct.
So, I think using `CFeeRate` is oka
...
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1675726351)
@glozow
> probably `FeeFrac` would be better? `CFeeRate` loses precision and can be inferred from the fee and size
We will be using the package size to calculate the percentile package and select its fee rate as an estimate to return to the user.
We then have to convert the fee and size to fee rate either by `CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes).GetFeePerK` or introduce and use a similar `GetFeeRate` method for the `FeeFrac` struct.
So, I think using `CFeeRate` is oka
...
💬 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?