💬 maflcko commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1410430170)
> accidentally introduce new stuff into the consensus lib.

Is it true? I don't think the list is enforced in any way. Nothing in C++ is holding anyone back from including a header with header-only code. For example, the current code includes headers that are not listed here:

```
src/consensus/tx_verify.cpp:#include <util/check.h>
src/consensus/tx_verify.cpp:#include <util/moneystr.h>
```

Also, it should be possible to include other headers without running into compile or link issues,
...

💬 maflcko commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1833464140)
Would there be any downside in going forward with https://github.com/bitcoin/bitcoin/pull/28851 instead, and then reverting it as part of this pull, once and if it is ready?

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1833502048)
Another one: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64593 cc @brunoerg

💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1410572606)
`tx_verify` is not part of the consensus library though.

💬 maflcko commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1410600302)
Ok, but the following diff on tx_check compiles for me, or am I missing something?


```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index b3fee1e8b1..1be8686bea 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -7,9 +7,14 @@
#include <consensus/amount.h>
#include <primitives/transaction.h>
#include <consensus/validation.h>
+#include <util/check.h>
+#include <util/fs.h>

bool CheckTransaction(const CTransaction& tx, TxVal
...

💬 aureleoules commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-1833714256)
The CoinSelection benchmarks fails when I run it with -min-time=1000
```
./src/bench/bench_bitcoin -filter=CoinSelection -min-time=1000
bench_bitcoin: bench/coin_selection.cpp:84: CoinSelection(ankerl::nanobench::Bench&)::<lambda()>: Assertion `result->GetSelectedValue() == 1003 * COIN' failed.
Aborted
```

👍 ryanofsky approved a pull request: "refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream"
(https://github.com/bitcoin/bitcoin/pull/28451#pullrequestreview-1757394287)
Seems odd to not code review ACK fa98a097a30bc39f2424c0efd28a7979155faae6 (looks good)

💬 brunoerg commented on pull request "test: add coverage for bech32m in `wallet_keypool_topup`":
(https://github.com/bitcoin/bitcoin/pull/28928#issuecomment-1833739551)
> This can be tested by loading and unloading wallets instead of restarting nodes (this was a restriction from the past, prev to introducing multi-wallets support). No extra nodes nor restart is needed.

Just addressed it here

💬 kashifs commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#issuecomment-1833744139)
@andrewtoth, okay thanks. I did as you suggested and was able to get similar speed improvements when testing the `rest` command. 10.260ms on branch `master` and 1.014ms on branch `bitcoin-read-raw-block`. I only see improvements when running the `rest` testing command more than once though. Is this the expected behavior? I hope it's not due to some caching or other improvements unrelated to your code updates. Also, I was not able to appreciate any improvements when running the `RPC` command. Cou
...

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1410643071)
> What's the issue with using the transaction's vsize

My reasons were:
- I wanted to specify a value of sigops that wouldn't depend on the node's `-bytespersigop` setting, and similarly didn't want it to be bypassable locally this way
- People expressed confusion at what sigops-adjusted vsize was, so I wanted to make it clearer

Happy to change though, if it's preferred?

💬 brunoerg commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1833748317)
> Another one: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64593 cc @brunoerg

I don't have access to see it in depth 🙁.

I suppose it might be related to the descriptor parse. I think we can use `ImportDescriptors` from `wallet_notifications` in `scriptpubkeyman`.

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1833752858)
> I don't have access to see it in depth 🙁.

The "Reproducer Testcase" should be public.

> I suppose it might be related to the descriptor parse.

Ah, good point. This was done intentionally. :sweat_smile:

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1410653204)
> Instead, I'd suggest that we not bother enforcing the v3 rules on a reorg.

I'm a fan of this approach. I want to spend a little bit of time thinking about what the possibilities are - I guess we can have some incorrect assumptions about transactions (resulting in e.g. incorect MinerScores) but it wouldn't be problematic if a reorg is the only way to get there, as you say.

💬 sipa commented on pull request "refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream":
(https://github.com/bitcoin/bitcoin/pull/28451#issuecomment-1833766618)
utACK fa98a097a30bc39f2424c0efd28a7979155faae6

💬 brunoerg commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1833771857)
> The "Reproducer Testcase" should be public.

"Access denied" when I try to access it.

🤔 furszy reviewed a pull request: "test: add coverage for bech32m in `wallet_keypool_topup`"
(https://github.com/bitcoin/bitcoin/pull/28928#pullrequestreview-1757470863)
how much faster the test got? Some numbers would encourage more reviewers to come here.

📝 maflcko opened a pull request: "test: Add and use option for tx-version in MiniWallet methods"
(https://github.com/bitcoin/bitcoin/pull/28972)
Fixes https://github.com/bitcoin/bitcoin/pull/26657#discussion_r1071362636 by adding a `version` keyword-argument to `create_self_transfer_multi`, and by passing it via `kwargs` from all other methods.

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1833845779)
Ok, I see. I guess google requires login now, starting at some point in the past.

💬 instagibbs commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#issuecomment-1833887261)
ping @Sjors @darosior @glozow should be ready for re-acks

💬 ajtowns commented on pull request "refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream":
(https://github.com/bitcoin/bitcoin/pull/28451#issuecomment-1833929465)
ACK fa98a097a30bc39f2424c0efd28a7979155faae6