💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049569201)
> (not sure what the fuzzer did, don't have access):
The issue I created includes the base64 encoded file that the fuzz engine on oss-fuzz produced
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049569201)
> (not sure what the fuzzer did, don't have access):
The issue I created includes the base64 encoded file that the fuzz engine on oss-fuzz produced
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2049571166)
1. The intent behind the code on master is to delete the nodes at specific points, otherwise `CNode::Release()` would do `if (--nRefCount == 0) delete this` (and had different logic around `m_nodes_disconnected`). You might be correct in that having been okay, nobody has been able to prove otherwise in this PR so far.
2. I agree that `shared_ptr`-usage in this PR not being exposed in the public interface of `CConnman` is a mitigating factor.
3. I bet you've checked all angles I would check
...
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2049571166)
1. The intent behind the code on master is to delete the nodes at specific points, otherwise `CNode::Release()` would do `if (--nRefCount == 0) delete this` (and had different logic around `m_nodes_disconnected`). You might be correct in that having been okay, nobody has been able to prove otherwise in this PR so far.
2. I agree that `shared_ptr`-usage in this PR not being exposed in the public interface of `CConnman` is a mitigating factor.
3. I bet you've checked all angles I would check
...
✅ l0rinc closed a pull request: "refactor: use original log string when no suspicious chars found"
(https://github.com/bitcoin/bitcoin/pull/31923)
(https://github.com/bitcoin/bitcoin/pull/31923)
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049582778)
This suggestion is not grammatically correct and doesn't make sense. The original is fine.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049582778)
This suggestion is not grammatically correct and doesn't make sense. The original is fine.
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049583367)
I don't think deduplicating this is really all that useful nor makes it easier to read.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049583367)
I don't think deduplicating this is really all that useful nor makes it easier to read.
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049584260)
I will leave this as is as there is no pubnonce object to tie the size to.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049584260)
I will leave this as is as there is no pubnonce object to tie the size to.
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588706)
Done
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588706)
Done
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588850)
Done
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588850)
Done
📝 instagibbs opened a pull request: "test: cover invalid codesep positions for signature in taproot"
(https://github.com/bitcoin/bitcoin/pull/32301)
I didn't find another spot where incorrect codesep coverage existed, so I added some and modified the serializer to allow for maximal value `0xfffffffe` since it was being serialized as a signed integer.
(https://github.com/bitcoin/bitcoin/pull/32301)
I didn't find another spot where incorrect codesep coverage existed, so I added some and modified the serializer to allow for maximal value `0xfffffffe` since it was being serialized as a signed integer.
💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609432)
Done
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609432)
Done
💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609523)
Done
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609523)
Done
💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609602)
Done
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609602)
Done
💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609663)
Done
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049609663)
Done
🤔 w0xlt reviewed a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2776864390)
Changes made to `rpc_rawtransaction.py` in https://github.com/bitcoin/bitcoin/pull/31936/commits/d0453049f5d6678096f3575f6d595467506a05f1 in the functions `raw_multisig_transaction_legacy_tests` and `raw_multisig_transaction_legacy_tests` does not have any effect.
A better approach might be to validate a v3 transaction in `createrawtransaction_tests`, for example, as shown below.
```diff
# Multiple mixed outputs
tx = tx_from_hex(self.nodes[2].createrawtransaction(inputs=[
...
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2776864390)
Changes made to `rpc_rawtransaction.py` in https://github.com/bitcoin/bitcoin/pull/31936/commits/d0453049f5d6678096f3575f6d595467506a05f1 in the functions `raw_multisig_transaction_legacy_tests` and `raw_multisig_transaction_legacy_tests` does not have any effect.
A better approach might be to validate a v3 transaction in `createrawtransaction_tests`, for example, as shown below.
```diff
# Multiple mixed outputs
tx = tx_from_hex(self.nodes[2].createrawtransaction(inputs=[
...
🤔 w0xlt reviewed a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953#pullrequestreview-2776921300)
ACK https://github.com/bitcoin/bitcoin/pull/31953/commits/fa86190e6ed2aeda7bcceaf96f52403816bcd751
CI error seems to be unrelated
(https://github.com/bitcoin/bitcoin/pull/31953#pullrequestreview-2776921300)
ACK https://github.com/bitcoin/bitcoin/pull/31953/commits/fa86190e6ed2aeda7bcceaf96f52403816bcd751
CI error seems to be unrelated
💬 sipa commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049668593)
I have added this in the unit tests:
```c++
// Test for integer overflow issue (https://github.com/bitcoin/bitcoin/issues/32294)
BOOST_CHECK_EQUAL(FeeFrac{0x7ffffffdfffffffb, 0x7ffffffd}.EvaluateFeeDown(0x7fffffff), 0x7fffffffffffffff);
```
But note that to trigger it, you need to build with sanitizers (which are generally only enabled in fuzz binaries, I think)?
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049668593)
I have added this in the unit tests:
```c++
// Test for integer overflow issue (https://github.com/bitcoin/bitcoin/issues/32294)
BOOST_CHECK_EQUAL(FeeFrac{0x7ffffffdfffffffb, 0x7ffffffd}.EvaluateFeeDown(0x7fffffff), 0x7fffffffffffffff);
```
But note that to trigger it, you need to build with sanitizers (which are generally only enabled in fuzz binaries, I think)?
🤔 fjahr reviewed a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2776864149)
Code review ACK f385ff7aba0eab6cf00ede1b808817dfd7047399
Typo may be fixed in the next MuSig2 PR unless you want to retouch.
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2776864149)
Code review ACK f385ff7aba0eab6cf00ede1b808817dfd7047399
Typo may be fixed in the next MuSig2 PR unless you want to retouch.
💬 fjahr commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049610253)
nit: s/particitpant/participant/
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049610253)
nit: s/particitpant/participant/
💬 galaxycores commented on pull request "bench: Fix WalletMigration benchmark":
(https://github.com/bitcoin/bitcoin/pull/32281#discussion_r2049674204)
AdToWallet(tx)
(https://github.com/bitcoin/bitcoin/pull/32281#discussion_r2049674204)
AdToWallet(tx)
💬 galaxycores commented on pull request "bench: Fix WalletMigration benchmark":
(https://github.com/bitcoin/bitcoin/pull/32281#discussion_r2049676386)
(WriteWachOnly;)
(https://github.com/bitcoin/bitcoin/pull/32281#discussion_r2049676386)
(WriteWachOnly;)