📝 ryanofsky opened a pull request: "multiprocess: add bitcoin-mine test program "
(https://github.com/bitcoin/bitcoin/pull/30437)
Add [`bitcoin-mine`](https://github.com/ryanofsky/bitcoin/blob/pr/mine/src/bitcoin-mine.cpp#L19-L42) executable to test connecting to the `bitcoin-node` process over a unix socket and calling [`interface::Mining`](https://github.com/ryanofsky/bitcoin/blob/pr/mine/src/interfaces/mining.h) methods.
This could be useful as discussed in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2222418506 to work on getting mining code working in an external process.
This PR is a draft, becaus
...
(https://github.com/bitcoin/bitcoin/pull/30437)
Add [`bitcoin-mine`](https://github.com/ryanofsky/bitcoin/blob/pr/mine/src/bitcoin-mine.cpp#L19-L42) executable to test connecting to the `bitcoin-node` process over a unix socket and calling [`interface::Mining`](https://github.com/ryanofsky/bitcoin/blob/pr/mine/src/interfaces/mining.h) methods.
This could be useful as discussed in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2222418506 to work on getting mining code working in an external process.
This PR is a draft, becaus
...
🤔 marcofleon reviewed a pull request: "fuzz: wallet: add target for `CreateTransaction`"
(https://github.com/bitcoin/bitcoin/pull/29936#pullrequestreview-2174428032)
Nice, coverage looks good now. The issue I'm seeing is the harness has really low stability. It's at 28% with the seed corpus I'm using. The only obvious source of randomness I'm seeing is in `CreateTransactionInternal`:
https://github.com/bitcoin/bitcoin/blob/4d6af61d879914a660e73db5c2f2e6c4d0aa8243/src/wallet/spend.cpp#L1164-L1166
Sometimes `changepos` isn't set so it does hit that line. Maybe there are other reasons for the low stability somewhere else?
(https://github.com/bitcoin/bitcoin/pull/29936#pullrequestreview-2174428032)
Nice, coverage looks good now. The issue I'm seeing is the harness has really low stability. It's at 28% with the seed corpus I'm using. The only obvious source of randomness I'm seeing is in `CreateTransactionInternal`:
https://github.com/bitcoin/bitcoin/blob/4d6af61d879914a660e73db5c2f2e6c4d0aa8243/src/wallet/spend.cpp#L1164-L1166
Sometimes `changepos` isn't set so it does hit that line. Maybe there are other reasons for the low stability somewhere else?
💬 hebasto commented on pull request "depends: bump boost to 1.85.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2225302383)
> The CI failure https://cirrus-ci.com/task/4950868422819840:
Maybe revert 3bee51427a05075150721f0a05ead8f92e1ba019?
FWIW, in the CMake staging branch, `set(CMAKE_PLATFORM_HAS_INSTALLNAME FALSE)` is used in between the `project()` and `enable_language(CXX)` calls to [workaround](https://github.com/hebasto/bitcoin/blob/292e7da439a732092e3823fadd2379bbb7d3ff2b/CMakeLists.txt#L45-L53) it:
```cmake
#=============================
# Language setup
#=============================
if(CMAKE_SYS
...
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2225302383)
> The CI failure https://cirrus-ci.com/task/4950868422819840:
Maybe revert 3bee51427a05075150721f0a05ead8f92e1ba019?
FWIW, in the CMake staging branch, `set(CMAKE_PLATFORM_HAS_INSTALLNAME FALSE)` is used in between the `project()` and `enable_language(CXX)` calls to [workaround](https://github.com/hebasto/bitcoin/blob/292e7da439a732092e3823fadd2379bbb7d3ff2b/CMakeLists.txt#L45-L53) it:
```cmake
#=============================
# Language setup
#=============================
if(CMAKE_SYS
...
🤔 maflcko reviewed a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2174408053)
left some comments
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2174408053)
left some comments
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675652818)
nit in a4ead4e51c04c75c7837fb53116062e79f502bbd (commit message): Could link directly to the discussion https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378 ?
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675652818)
nit in a4ead4e51c04c75c7837fb53116062e79f502bbd (commit message): Could link directly to the discussion https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378 ?
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675665249)
The compiler will generate those, no? So seems fine to remove?
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675665249)
The compiler will generate those, no? So seems fine to remove?
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675658574)
Both sides are trimmed. My recommendation would be to drop the first commit and instead add a unit test:
```diff
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
index b7892a2139..5602b33e17 100644
--- a/src/test/uint256_tests.cpp
+++ b/src/test/uint256_tests.cpp
@@ -102,6 +102,8 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
BOOST_CHECK(uint256S("0x"+MaxL.ToString()) == MaxL);
BOOST_CHECK(uint256S(R1L.ToString()) == R1L);
BO
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675658574)
Both sides are trimmed. My recommendation would be to drop the first commit and instead add a unit test:
```diff
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
index b7892a2139..5602b33e17 100644
--- a/src/test/uint256_tests.cpp
+++ b/src/test/uint256_tests.cpp
@@ -102,6 +102,8 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
BOOST_CHECK(uint256S("0x"+MaxL.ToString()) == MaxL);
BOOST_CHECK(uint256S(R1L.ToString()) == R1L);
BO
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675665868)
Same (the compiler will generate those)?
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675665868)
Same (the compiler will generate those)?
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675666153)
Also, the same for `inline uint160 uint160S(`.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675666153)
Also, the same for `inline uint160 uint160S(`.
💬 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_r1675668215)
Are we going to upstream this? Suprised there's not already an option to do this for at least `date_time`, given it's a stub library that only exists for back compat. Wonder if we can (easily) patch it out in the interim.
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1675668215)
Are we going to upstream this? Suprised there's not already an option to do this for at least `date_time`, given it's a stub library that only exists for back compat. Wonder if we can (easily) patch it out in the interim.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675668347)
Also, could add a unit test that truncated input is parsed? (Sad as well, but again existing behavior, so probably good to keep for now)
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675668347)
Also, could add a unit test that truncated input is parsed? (Sad as well, but again existing behavior, so probably good to keep for now)
💬 fanquake commented on pull request "depends: bump boost to 1.85.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2225307394)
> Maybe revert https://github.com/bitcoin/bitcoin/commit/3bee51427a05075150721f0a05ead8f92e1ba019?
A better solution would be to fix the CMake build systems, so we don't have to pointlessly compile stub libraries, that we ultimately don't even use.
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2225307394)
> Maybe revert https://github.com/bitcoin/bitcoin/commit/3bee51427a05075150721f0a05ead8f92e1ba019?
A better solution would be to fix the CMake build systems, so we don't have to pointlessly compile stub libraries, that we ultimately don't even use.
💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2225329321)
> The only obvious source of randomness I'm seeing is in `CreateTransactionInternal`:
Does it improve if you call `SeedRandomForTest` before every execution of every fuzz input?
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2225329321)
> The only obvious source of randomness I'm seeing is in `CreateTransactionInternal`:
Does it improve if you call `SeedRandomForTest` before every execution of every fuzz input?
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675687795)
(same commit): Also, you'd have to fix `WtxidFromString(` in the same commit?
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675687795)
(same commit): Also, you'd have to fix `WtxidFromString(` in the same commit?
💬 ryanofsky commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100)
re: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225041208
> I don't think this can be solved with making both calls at the same time; the second call needs to reference a specific template that the first call generated.
A natural way to do this would be to have `createNewBlock` return an interface instead of a struct:
```c++
class Mining
{
virtual std::unique_ptr<MiningBlock> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
}
class
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100)
re: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225041208
> I don't think this can be solved with making both calls at the same time; the second call needs to reference a specific template that the first call generated.
A natural way to do this would be to have `createNewBlock` return an interface instead of a struct:
```c++
class Mining
{
virtual std::unique_ptr<MiningBlock> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
}
class
...
💬 maflcko commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2225345559)
> , so I think it needs to be something inside the docker container. Also, I am not aware of a process inside the CI env that would echo back what it got.
Also interesting that no one reproduced this locally yet
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2225345559)
> , so I think it needs to be something inside the docker container. Also, I am not aware of a process inside the CI env that would echo back what it got.
Also interesting that no one reproduced this locally yet
✅ willcl-ark closed a pull request: "policy: Enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/28132)
(https://github.com/bitcoin/bitcoin/pull/28132)
💬 willcl-ark commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2225369634)
Thank everyone for your participation in this discussion and review of this pull request so far.
However the comments section here has become difficult to follow due to numerous off-topic comments, a few personal disagreements, and repetition of arguments.
In the interest of having a more productive and focused technical and philosophical discussion we are going to close and lock this PR.
-----
@petertodd: We strongly encourage you to open a new pull request for this proposal, as the
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2225369634)
Thank everyone for your participation in this discussion and review of this pull request so far.
However the comments section here has become difficult to follow due to numerous off-topic comments, a few personal disagreements, and repetition of arguments.
In the interest of having a more productive and focused technical and philosophical discussion we are going to close and lock this PR.
-----
@petertodd: We strongly encourage you to open a new pull request for this proposal, as the
...
📝 fanquake locked a pull request: "policy: Enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/28132)
The following pools are have enabled full-rbf:
- [Foundry USA, 30%](https://mempool.space/tx/ca2ed3ea924131d4051569c9c59dc62191020674ba9b89606c3a725eefbc955b)
- [Antpool, 28%](https://mempool.space/tx/53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643)
- [ViaBTC, 14%](https://mempool.space/tx/4381156979261d6d46af74e0bc0553d24787bad9b3f2301fa9c9ba7c56e27bd3)
- [F2Pool, 10%](https://mempool.space/tx/e1213b60f1ac93b412a992deb0a79eebbde0032179c545350cc2136d0a3754fb)
- [Binance P
...
(https://github.com/bitcoin/bitcoin/pull/28132)
The following pools are have enabled full-rbf:
- [Foundry USA, 30%](https://mempool.space/tx/ca2ed3ea924131d4051569c9c59dc62191020674ba9b89606c3a725eefbc955b)
- [Antpool, 28%](https://mempool.space/tx/53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643)
- [ViaBTC, 14%](https://mempool.space/tx/4381156979261d6d46af74e0bc0553d24787bad9b3f2301fa9c9ba7c56e27bd3)
- [F2Pool, 10%](https://mempool.space/tx/e1213b60f1ac93b412a992deb0a79eebbde0032179c545350cc2136d0a3754fb)
- [Binance P
...
📝 fanquake opened a pull request: "guix: (explicitly) build Linux GCC with `--enable-cet`"
(https://github.com/bitcoin/bitcoin/pull/30438)
Similar to #29695, and in the same vein of explicitly configuring hardening options in our release toolchain.
See https://gcc.gnu.org/install/configure.html:
>` --enable-cet`
> Enable building target run-time libraries with control-flow instrumentation, see `-fcf-protection option`. When --enable-cet is specified target libraries are configured to add `-fcf-protection` and, if needed, other target specific options to a set of building options.
> `--enable-cet=auto` is default. CET is
...
(https://github.com/bitcoin/bitcoin/pull/30438)
Similar to #29695, and in the same vein of explicitly configuring hardening options in our release toolchain.
See https://gcc.gnu.org/install/configure.html:
>` --enable-cet`
> Enable building target run-time libraries with control-flow instrumentation, see `-fcf-protection option`. When --enable-cet is specified target libraries are configured to add `-fcf-protection` and, if needed, other target specific options to a set of building options.
> `--enable-cet=auto` is default. CET is
...