💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1410161736)
yay! looks pythonic! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1410161807)
done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1410161873)
good suggestion! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1410162030)
missed that! i've updated the subtest in `rpc_net.py`.

💬 casey commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1833205912)
There are a few issues with this PR.

- By changing the default, instead of being opt in, it represents a
potentially disruptive and unwelcome change in behavior for miners relying
on Bitcoin Core to construct block templates.

Over the last ten months, inscription reveal transactions have paid at least
$121,386,535 in transaction fees. (Or rasan-titon-vybong, timill-bysanditonra
dollars, for those who prefer the tonal number system.)

Miners who use an updated version of B
...

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1833222343)
thanks @theStack, @mzumsande! I've updated the PR to address your comments.
Also made this change to `MSGTYPE_TO_SHORTID` construction because of `lint-python.py` failure in the CI

```diff
diff --git a/test/functional/test_framework/v2_p2p.py b/test/functional/test_framework/v2_p2p.py
index fe44351a13..b4efd1ac58 100644
--- a/test/functional/test_framework/v2_p2p.py
+++ b/test/functional/test_framework/v2_p2p.py
@@ -55,7 +55,7 @@ SHORTID = {
}

# Dictionary which contains short me
...

💬 maflcko commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1410414848)
unrelated: What is the point of having headers in here? Wouldn't it be better to move them all to the CORE_H list, like for all other libraries (`common/*.h`, `util/*.h`, `node/*.h`, ...)?

💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1410419017)
That point is that you can compile a more minimal `libbitcoin_consensus` without having to drag in the full `BITCOIN_CORE_H` and accidentally introduce new stuff into the consensus lib.

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