🚀 achow101 merged a pull request: "Refactor: Redefine CTransaction equality to include witness data"
(https://github.com/bitcoin/bitcoin/pull/32723)
(https://github.com/bitcoin/bitcoin/pull/32723)
💬 davidgumberg commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2178626637)
```suggestion
for (const auto& [coin, persistent] : m_locked_coins) {
if (!success) break;
if (persistent) success = batch.EraseLockedUTXO(coin);
}
m_locked_coins.clear();
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2178626637)
```suggestion
for (const auto& [coin, persistent] : m_locked_coins) {
if (!success) break;
if (persistent) success = batch.EraseLockedUTXO(coin);
}
m_locked_coins.clear();
💬 achow101 commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3025700501)
In be3435c2ff82f69db11ba032452614404dac15f9 "bench: make ObfuscationBench more representative", the commit message states
> Since a previous PR already solved the tiny byte-array xors during serialization, we're only concentrating on big continuous chunks now.
and the commit essentially removes the previous benchmark. However, I don't think that's a good justification for removing the benchmark. Benchmarks for things that are "resolved" should still be kept around in order to check for reg
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3025700501)
In be3435c2ff82f69db11ba032452614404dac15f9 "bench: make ObfuscationBench more representative", the commit message states
> Since a previous PR already solved the tiny byte-array xors during serialization, we're only concentrating on big continuous chunks now.
and the commit essentially removes the previous benchmark. However, I don't think that's a good justification for removing the benchmark. Benchmarks for things that are "resolved" should still be kept around in order to check for reg
...
💬 Ronlabrie7470 commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-3026129000)
> Currently the *.dat files in the blocksdir store the data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan them and move them into quarantine, or delete them, or corrupt them. This may cause Bitcoin Core to fail a reorg, or fail to reply to block requests (via P2P, RPC, REST, ...).
>
> Fix this, similar to https://github.com/bitcoin/bitcoin/pull/665
...
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-3026129000)
> Currently the *.dat files in the blocksdir store the data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan them and move them into quarantine, or delete them, or corrupt them. This may cause Bitcoin Core to fail a reorg, or fail to reply to block requests (via P2P, RPC, REST, ...).
>
> Fix this, similar to https://github.com/bitcoin/bitcoin/pull/665
...
💬 Hameedoooooo commented on pull request "test: check P2SH sigop count for coinbase tx":
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3026362021)
1KQDpaSSdjezuBMVzZtAScuav8KKBtjchM
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3026362021)
1KQDpaSSdjezuBMVzZtAScuav8KKBtjchM
💬 Hameedoooooo commented on pull request "[WIP] wallet: tx creation, don't select outputs from txes that are being replaced":
(https://github.com/bitcoin/bitcoin/pull/26732#issuecomment-3026393632)
1KQDpaSSdjezuBMVzZtAScuav8KKBtjchM
(https://github.com/bitcoin/bitcoin/pull/26732#issuecomment-3026393632)
1KQDpaSSdjezuBMVzZtAScuav8KKBtjchM
💬 maflcko commented on issue "Internal bug detected: FinalizeAndExtractPSBT(psbtx_copy, mtx)":
(https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3026550370)
uploaded to oss-fuzz (https://issues.oss-fuzz.com/issues/429003360) to get a regression range: https://github.com/bitcoin/bitcoin/compare/b000ed5ee54438efb72e190262faeb535489d35d...ac9fa6ec78158e41006d621ee62e44dec0f934c0 (exists since the beginning, when the line was added)
(https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3026550370)
uploaded to oss-fuzz (https://issues.oss-fuzz.com/issues/429003360) to get a regression range: https://github.com/bitcoin/bitcoin/compare/b000ed5ee54438efb72e190262faeb535489d35d...ac9fa6ec78158e41006d621ee62e44dec0f934c0 (exists since the beginning, when the line was added)
💬 Eunovo commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3026599126)
ACK https://github.com/bitcoin/bitcoin/pull/32618/commits/b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3026599126)
ACK https://github.com/bitcoin/bitcoin/pull/32618/commits/b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179306743)
Try `ConsumeIntegralInRange(0, std::numeric_limits<int32_t>::max())`
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179306743)
Try `ConsumeIntegralInRange(0, std::numeric_limits<int32_t>::max())`
💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179291875)
https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374: Consider
```diff
diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp
index 40f3119cf1..65278aaa41 100644
--- a/src/test/amount_tests.cpp
+++ b/src/test/amount_tests.cpp
@@ -78,13 +78,15 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
BOOST_CHECK(CFeeRate(CAmount(-1), 1000) == CFeeRate(-1));
BOOST_CHECK(CFeeRate(CAmount(0), 1000) == CFeeRate(0));
BOOST_CHECK(CFeeRate(CAmoun
...
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179291875)
https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374: Consider
```diff
diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp
index 40f3119cf1..65278aaa41 100644
--- a/src/test/amount_tests.cpp
+++ b/src/test/amount_tests.cpp
@@ -78,13 +78,15 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
BOOST_CHECK(CFeeRate(CAmount(-1), 1000) == CFeeRate(-1));
BOOST_CHECK(CFeeRate(CAmount(0), 1000) == CFeeRate(0));
BOOST_CHECK(CFeeRate(CAmoun
...
💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179302609)
https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374:
```diff
diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp
index 20578c325e..26146bc617 100644
--- a/src/test/fuzz/fee_rate.cpp
+++ b/src/test/fuzz/fee_rate.cpp
@@ -20,8 +20,8 @@ FUZZ_TARGET(fee_rate)
const CFeeRate fee_rate{satoshis_per_k};
(void)fee_rate.GetFeePerK();
- const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();
- if (bytes >= 0
...
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179302609)
https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374:
```diff
diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp
index 20578c325e..26146bc617 100644
--- a/src/test/fuzz/fee_rate.cpp
+++ b/src/test/fuzz/fee_rate.cpp
@@ -20,8 +20,8 @@ FUZZ_TARGET(fee_rate)
const CFeeRate fee_rate{satoshis_per_k};
(void)fee_rate.GetFeePerK();
- const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();
- if (bytes >= 0
...
💬 maflcko commented on something "":
(https://github.com/bitcoin/bitcoin/commit/be3435c2ff82f69db11ba032452614404dac15f9#r161257474)
nit in be3435c2ff82f69db11ba032452614404dac15f9: Any reason to rename `data` to `test_data` and change the unit? Seems fine to just keep `bench.batch(data.size()).unit("byte").run([&] {`?
(https://github.com/bitcoin/bitcoin/commit/be3435c2ff82f69db11ba032452614404dac15f9#r161257474)
nit in be3435c2ff82f69db11ba032452614404dac15f9: Any reason to rename `data` to `test_data` and change the unit? Seems fine to just keep `bench.batch(data.size()).unit("byte").run([&] {`?
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3026764342)
> removing the benchmark
My understanding is that changing the benchmark is required. Maybe this can be explained better in the commit message to mention:
* 31-byte xor-keys are not used in the codebase, so using the common size (8) makes more sense.
I am not sure about changing the size to 10_MiB, because database obfuscation still runs on the values individually, which are smaller in reality. Also, the buffered reader will read smaller chunks than that in reality. So maybe the data si
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3026764342)
> removing the benchmark
My understanding is that changing the benchmark is required. Maybe this can be explained better in the commit message to mention:
* 31-byte xor-keys are not used in the codebase, so using the common size (8) makes more sense.
I am not sure about changing the size to 10_MiB, because database obfuscation still runs on the values individually, which are smaller in reality. Also, the buffered reader will read smaller chunks than that in reality. So maybe the data si
...
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2179333243)
nit in be3435c2ff82f69db11ba032452614404dac15f9: Any reason to rename `data` to `test_data` and change the unit? Seems fine to just keep `bench.batch(data.size()).unit("byte").run([&] {`?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2179333243)
nit in be3435c2ff82f69db11ba032452614404dac15f9: Any reason to rename `data` to `test_data` and change the unit? Seems fine to just keep `bench.batch(data.size()).unit("byte").run([&] {`?
💬 maflcko commented on pull request "test: check P2SH sigop count for coinbase tx":
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3026825528)
the branch is dead code outside of tests, so it could also be removed/disabled. Though, it may be better to add a test than to refactor the consensus code here. So lgtm
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3026825528)
the branch is dead code outside of tests, so it could also be removed/disabled. Though, it may be better to add a test than to refactor the consensus code here. So lgtm
⚠️ maflcko opened an issue: "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds"
(https://github.com/bitcoin/bitcoin/issues/32855)
in `self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True)`,
https://cirrus-ci.com/task/5704849158832128?logs=ci#L2967:
```
node1 2025-06-26T14:05:40.072184Z [msghand] [net_processing.cpp:3452] [ProcessMessage] [net] received: pong (8 bytes) peer=0
test 2025-06-26T14:05:43.222938Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_contai
...
(https://github.com/bitcoin/bitcoin/issues/32855)
in `self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True)`,
https://cirrus-ci.com/task/5704849158832128?logs=ci#L2967:
```
node1 2025-06-26T14:05:40.072184Z [msghand] [net_processing.cpp:3452] [ProcessMessage] [net] received: pong (8 bytes) peer=0
test 2025-06-26T14:05:43.222938Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_contai
...
💬 maflcko commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3026946485)
re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 🌈
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK b1a8ac07e91dd1d305fc
...
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3026946485)
re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 🌈
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK b1a8ac07e91dd1d305fc
...
💬 rkrux commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2179522247)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2179522247)
Thanks, fixed.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3027077942)
[Posted](https://gnusha.org/pi/bitcoindev/49dyqqkf5NqGlGdinp6SELIoxzE_ONh3UIj6-EB8S804Id5yROq-b1uGK8DUru66eIlWuhb5R3nhRRutwuYjemiuOOBS2FQ4KWDnEh0wLuA=@protonmail.com/T/#u) about this to the mailing list this morning.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3027077942)
[Posted](https://gnusha.org/pi/bitcoindev/49dyqqkf5NqGlGdinp6SELIoxzE_ONh3UIj6-EB8S804Id5yROq-b1uGK8DUru66eIlWuhb5R3nhRRutwuYjemiuOOBS2FQ4KWDnEh0wLuA=@protonmail.com/T/#u) about this to the mailing list this morning.
💬 darosior commented on pull request "test: check P2SH sigop count for coinbase tx":
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3027122136)
Yeah, i think it's preferable to remove dead code than to test it:
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 95466b759cb..5036e26e21a 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -125,8 +125,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& inputs)
{
- if (tx.IsCoinBase())
- return 0;
+ Assert(!tx.IsCoin
...
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3027122136)
Yeah, i think it's preferable to remove dead code than to test it:
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 95466b759cb..5036e26e21a 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -125,8 +125,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& inputs)
{
- if (tx.IsCoinBase())
- return 0;
+ Assert(!tx.IsCoin
...