💬 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(`.

💬 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.

💬 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)

💬 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.

💬 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?

💬 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?

💬 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
...

💬 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

✅ willcl-ark closed a pull request: "policy: Enable full-rbf by default"
(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
...

📝 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
...

📝 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
...

👍 stickies-v approved a pull request: "rpc: Use CHECK_NONFATAL over Assert"
(https://github.com/bitcoin/bitcoin/pull/30429#pullrequestreview-2174514321)
ACK fa6270737eb9655bfb4e29b7070ecb6cd2087b7f

💬 stickies-v commented on pull request "rpc: Use CHECK_NONFATAL over Assert":
(https://github.com/bitcoin/bitcoin/pull/30429#discussion_r1675717508)
> If you want to be more accurate you'll have to write a clang-tidy plugin, but I won't be doing this myself.

The issue doesn't seem very likely to happen, so I don't think we should spend too much time on this indeed, but since clang-format doesn't red the CI I feel like not removing two characters is a sensible approach to catch this. Either way is fine for me, so up to you.

> Keep in mind that macOS is not supported.

Ugh, sorry didn't consider that. I confirmed that the new test work
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225384531)
> Implementation of `MiningBlock` could own all the data it needs and release it when it is destroyed.

Thanks, that makes sense.

> I could drop the commits in https://github.com/bitcoin/bitcoin/pull/30437 related to allowing a spawned process to detach and not exit when the last IPC connection is closed.

I think that's a good idea. Will study the PR.

💬 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
...

💬 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.

💬 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?

💬 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
...

💬 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)
```