💬 theuni commented on pull request "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256":
(https://github.com/bitcoin/bitcoin/pull/29180#discussion_r1442077985)
Updated for @fanquake's request: it's now exclusively used to control libbitcoinconsensus exports. libbitcoinkernel will get a new define when the time comes.

📝 L0laL33tz opened a pull request: "test: randomize perturbed files excluding ldb"
(https://github.com/bitcoin/bitcoin/pull/29182)
As discussed in #28831 the ldb files are too small to be randomized for pertubation. This PR excludes ldb files from randomization. Blockfiles are randomly perturbed, ldb file pertubation remains deterministic. For additional rationale see #28612

🤔 ismaelsadeeq reviewed a pull request: "rpc: validate fee estimation mode case insensitive"
(https://github.com/bitcoin/bitcoin/pull/29175#pullrequestreview-1804697537)
Concept ACK

Thanks for trying to solve this issue.

This patch does not fix the issue.

Behaviour on master 65c05db660b2ca1d0076b0d8573a6760b3228068
```terminal
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -fallbackfee=0.0000100 -daemon
Bitcoin Core starting
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
{
"name": "abubakar-test"
}
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -reg
...

💬 theuni commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1877527471)
Any reason not to do `Rotl` in sha3 and `ROTL` in siphash as well?

💬 jonatack commented on pull request "Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes":
(https://github.com/bitcoin/bitcoin/pull/29050#discussion_r1442123784)
A working build and green CI may help attract review. The last commit https://github.com/bitcoin/bitcoin/pull/29050/commits/c368d32588ff79efc189fbdcfe559d1ca081c36c doesn't build for me without updating the related fuzz tests.

<details><summary>sample diff</summary><p>

```diff
diff --git a/src/test/fuzz/script_assets_test_minimizer.cpp b/src/test/fuzz/script_assets_test_minimizer.cpp
index 511b581f606..7478897e170 100644
--- a/src/test/fuzz/script_assets_test_minimizer.cpp
+++ b/src/t
...

🤔 1ma reviewed a pull request: "datacarriersize: Match more datacarrying"
(https://github.com/bitcoin/bitcoin/pull/28408#pullrequestreview-1804807600)
In order to be consistent with the documentation of `datacarriersize`, this PR needs to roll back the changes recently made in `src/init.cpp` by this other PR: https://github.com/bitcoin/bitcoin/pull/27832

Not sure about the tests it added.

🤔 furszy reviewed a pull request: "Compressed Bitcoin Transactions"
(https://github.com/bitcoin/bitcoin/pull/29134#pullrequestreview-1804829839)
You could run the lint whitespace task locally. It is inside test/lint folder.

💬 0xB10C commented on pull request "build: remove systemtap variadic patch":
(https://github.com/bitcoin/bitcoin/pull/29181#issuecomment-1877675266)
ACK. That patch isn't needed anymore.

💬 brunoerg commented on pull request "fuzz: set `nMaxOutboundLimit` in connman target":
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1877676278)
friendly ping: @dergoegge

💬 dzyphr commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877684914)
> > > Concept ACK
> > > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
> >
> >
> > As long as miners continue to mine these transa
...

💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `void(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-1877688376)
> 1 - rename "void(KEY)" to "unused(KEY)"
> 3 - add logic to `importdescriptors` to disallow importing an unused(KEY) if KEY is used by another descriptor.

Done these. I've also added a further restriction that unused() cannot be import to a wallet without private keys. It isn't useful in such wallets so I think it makes sense to disallow their import.

> 2 - add logic to `importdescriptors` and `generatewalletdescriptor` to delete any unused(KEY) descriptor as soon as the KEY which it r
...

💬 hebasto commented on pull request "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256":
(https://github.com/bitcoin/bitcoin/pull/29180#issuecomment-1877692626)
Concept ACK.

It makes the `libbitcoinkernel` code equivalent for both internal and external usage.

🤔 luke-jr requested changes to a pull request: "wallet, rpc: add BIP44 `account` in `createwallet`"
(https://github.com/bitcoin/bitcoin/pull/29129#pullrequestreview-1804913032)
Beyond wallet_name, none of the positional parameters make sense as positional, and that goes for this new one too. Migrating this to an options object should probably be done sooner rather than after making the problem worse

🤔 hebasto reviewed a pull request: "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256"
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1804912723)
In the master branch, the following code: https://github.com/bitcoin/bitcoin/blob/dcfb9ee9f6c8ce1f8c1e5dffdb207e55098ee9c5/src/crypto/sha256.cpp#L635-L640 compiles regardless of the `BUILD_BITCOIN_INTERNAL` macro. Now it is gated with the `DISABLE_OPTIMIZED_SHA256` one. Is it intentional?

💬 hebasto commented on pull request "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256":
(https://github.com/bitcoin/bitcoin/pull/29180#discussion_r1442214009)
nit:
```suggestion
#endif // DISABLE_OPTIMIZED_SHA256
```

💬 darosior commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1442215861)
Interesting. I was aware of `open_memstream` but not `memfd_create`. It's slower but could actually be helpful to get rid of the `128 MiB` allocs and make it possible to reindex.

💬 luke-jr commented on pull request "bitcoin-cli help detail to show full help for all RPCs":
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1877715274)
Concept NACK on the top commit: having each one in a separate file is nice.

⚠️ furszy opened an issue: "Prune Node Rescan Project Tracking"
(https://github.com/bitcoin/bitcoin/issues/29183)
This issue will be edited frequently to reflect the current status of the project.

#### What should I review now?

👇 👇 👇 👇 👇 👇
#28170 and #28955
☝️ ☝️ ☝️ ☝️ ☝️ ☝️


The goal of this project is to enable complete blockchain rescan capability on prune nodes, allowing these storage-limited nodes to import new wallet descriptors and update existing ones at any time, as well as loading external wallets with a birth-time older than 48 hours. This, in turn, enables the monitoring of
...

💬 achow101 commented on pull request "wallet, rpc: add BIP44 `account` in `createwallet`":
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1877735190)
> You mean have a specific RPC for external signers or extending `createwalletdescriptor`?

Unsure of the final design, but something like `createwalletdescriptor` for external signers seems like it would be useful, and having an account argument in that would probably also make sense. Actually, that might make sense for `createwalletdescriptor` itself.

💬 hebasto commented on pull request "util: remove Boost posix_time usage from GetTime*":
(https://github.com/bitcoin/bitcoin/pull/21110#issuecomment-1877739664)
@fanquake
> I have a followup that should remove the last of our `boost:posix_time` usage in `ParseISO8601DateTime`...

> I'm not sure that waiting the many years until the project will actually be able to use C++20...

As `std::chrono::parse` is available now, do you mind submitting your follow up? If not, I can do it :)