📝 l0rinc opened a pull request: "refactor: sort ubsan suppression values for consistency"
(https://github.com/bitcoin/bitcoin/pull/32298)
(https://github.com/bitcoin/bitcoin/pull/32298)
✅ l0rinc closed a pull request: "refactor: sort ubsan suppression values for consistency"
(https://github.com/bitcoin/bitcoin/pull/32298)
(https://github.com/bitcoin/bitcoin/pull/32298)
💬 l0rinc commented on pull request "refactor: sort ubsan suppression values for consistency":
(https://github.com/bitcoin/bitcoin/pull/32298#issuecomment-2813118238)
Wrong repo again, sorry
(https://github.com/bitcoin/bitcoin/pull/32298#issuecomment-2813118238)
Wrong repo again, sorry
📝 fanquake opened a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32299)
Backports:
- #32070
- #32187
- #32286
(https://github.com/bitcoin/bitcoin/pull/32299)
Backports:
- #32070
- #32187
- #32286
💬 laanwj commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2049097617)
Passing `-rpcconnect` and `-ipcconnect` at the same time should probably result in an error.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2049097617)
Passing `-rpcconnect` and `-ipcconnect` at the same time should probably result in an error.
💬 laanwj commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2813169967)
Concept ACK, it'd be useful to have an utility that can talk to the node through IPC and making `bitcoin-cli` double for that prevents code duplication.
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2813169967)
Concept ACK, it'd be useful to have an utility that can talk to the node through IPC and making `bitcoin-cli` double for that prevents code duplication.
🤔 rkrux reviewed a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2775871072)
I had verified the implementation with the spec mentioned in [BIP 373](https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki) & had navigated through the data stream serialization functions used internally when I reviewed the PR last time but forgot to mention in my review - thanks for implementing the suggested changes in previous review. Though I feel there is still some duplication & hardcoded values that can be removed and I would be quick to review/ack if they too are done.
From
...
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2775871072)
I had verified the implementation with the spec mentioned in [BIP 373](https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki) & had navigated through the data stream serialization functions used internally when I reviewed the PR last time but forgot to mention in my review - thanks for implementing the suggested changes in previous review. Though I feel there is still some duplication & hardcoded values that can be removed and I would be quick to review/ack if they too are done.
From
...
💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049062217)
To get rid of `66` here, I don't prefer using `2 * CPubKey::COMPRESSED_SIZE` and instead would prefer a constant for nonce that could be used later in this commit (part of draft PR) as well: https://github.com/bitcoin/bitcoin/pull/29675/commits/59612a514e348c2c3a8d2fbb82ac8245b139febc
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049062217)
To get rid of `66` here, I don't prefer using `2 * CPubKey::COMPRESSED_SIZE` and instead would prefer a constant for nonce that could be used later in this commit (part of draft PR) as well: https://github.com/bitcoin/bitcoin/pull/29675/commits/59612a514e348c2c3a8d2fbb82ac8245b139febc
💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049070414)
This portion is completely same as for the inputs one but the whole `if` block can be deduplicated by using passing `input/output` & `in/out` as args to common function.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049070414)
This portion is completely same as for the inputs one but the whole `if` block can be deduplicated by using passing `input/output` & `in/out` as args to common function.
💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049032764)
I realise that my previous review didn't mention to remove all instances clearly, though that's what I had in mind.
There are few instances of `33` and `34` left, I feel now that we are using `CPubKey::COMPRESSED_SIZE` in few places already, we might as well use it in the whole commit. I don't see a reason to keep remaining instances of hardcoded values lying around.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049032764)
I realise that my previous review didn't mention to remove all instances clearly, though that's what I had in mind.
There are few instances of `33` and `34` left, I feel now that we are using `CPubKey::COMPRESSED_SIZE` in few places already, we might as well use it in the whole commit. I don't see a reason to keep remaining instances of hardcoded values lying around.
💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049044328)
This portion is exactly same as the one used in the nonces case, can be extracted into a `DeserializeMuSig2PartialSigOrPubNonceKey` function because the format of the key is same in both the cases.
Though the duplication is not enough but the deduplicated version would enforce the similarity of the key format to the reader easily.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049044328)
This portion is exactly same as the one used in the nonces case, can be extracted into a `DeserializeMuSig2PartialSigOrPubNonceKey` function because the format of the key is same in both the cases.
Though the duplication is not enough but the deduplicated version would enforce the similarity of the key format to the reader easily.
💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049074691)
Nit here and in the `musig2_partial_sigs` section below.
```diff
- {RPCResult::Type::STR_HEX, "aggregate_pubkey", "The compressed aggregate public key for which this pubnonce is for."},
+ {RPCResult::Type::STR_HEX, "aggregate_pubkey", "The compressed aggregate public key for which this pubnonce is."},
```
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049074691)
Nit here and in the `musig2_partial_sigs` section below.
```diff
- {RPCResult::Type::STR_HEX, "aggregate_pubkey", "The compressed aggregate public key for which this pubnonce is for."},
+ {RPCResult::Type::STR_HEX, "aggregate_pubkey", "The compressed aggregate public key for which this pubnonce is."},
```
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2049127611)
Just noting here that this is also racy (like reattempt). If we have more than 1 connection open and more than 1 succeeds, it will decrement the extra successful connections count.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2049127611)
Just noting here that this is also racy (like reattempt). If we have more than 1 connection open and more than 1 succeeds, it will decrement the extra successful connections count.
💬 ismaelsadeeq commented on issue "Test Package Accept":
(https://github.com/bitcoin/bitcoin/issues/32160#issuecomment-2813204991)
> There is already no requirement of connectedness now
But it is currently enforced on master, this test fail
<details>
<summary>diff</summary>
```diff
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
index a2f9210f94d..807ede343a5 100755
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -88,6 +88,7 @@ class RPCPackagesTest(BitcoinTestFramework):
self.test_multiple_parents()
self.test_conflicting()
self.test_
...
(https://github.com/bitcoin/bitcoin/issues/32160#issuecomment-2813204991)
> There is already no requirement of connectedness now
But it is currently enforced on master, this test fail
<details>
<summary>diff</summary>
```diff
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
index a2f9210f94d..807ede343a5 100755
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -88,6 +88,7 @@ class RPCPackagesTest(BitcoinTestFramework):
self.test_multiple_parents()
self.test_conflicting()
self.test_
...
💬 Sjors commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813205001)
utACK ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
If this lands before #31802 then that needs to be updated to make this the default instruction.
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813205001)
utACK ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
If this lands before #31802 then that needs to be updated to make this the default instruction.
👍 laanwj approved a pull request: "doc: Add deps install notes for multiprocess"
(https://github.com/bitcoin/bitcoin/pull/32293#pullrequestreview-2776074231)
ACK ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
This needs to be mentioned. i think adding paragraph after paragraph of dependencies to the build docs like this becomes a bit of a mess for someone without context "what do i actually have to install", but that's not something that needs to be resolved here.
(https://github.com/bitcoin/bitcoin/pull/32293#pullrequestreview-2776074231)
ACK ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
This needs to be mentioned. i think adding paragraph after paragraph of dependencies to the build docs like this becomes a bit of a mess for someone without context "what do i actually have to install", but that's not something that needs to be resolved here.
💬 sipa commented on issue "Wallet should be able to store multiple transactions with same txid":
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813238080)
I think this really calls for storing up to 2 variants of each transactions: the one we expect to confirm, and optionally the one we've seen accepted in a block.
Super hacky way of avoiding database upgrade issues: store inside the `wtx` record a concatenation of both serialized transactions. Old wallets (I think) will just read the first one, ignoring the second version if any.
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813238080)
I think this really calls for storing up to 2 variants of each transactions: the one we expect to confirm, and optionally the one we've seen accepted in a block.
Super hacky way of avoiding database upgrade issues: store inside the `wtx` record a concatenation of both serialized transactions. Old wallets (I think) will just read the first one, ignoring the second version if any.
💬 Sjors commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2813323000)
All interface changes that I need for v30 have been merged!
Some progress has been made on enabling IPC for the release as well, but please review #31375 and https://github.com/bitcoin/bitcoin/pull/31802.
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2813323000)
All interface changes that I need for v30 have been merged!
Some progress has been made on enabling IPC for the release as well, but please review #31375 and https://github.com/bitcoin/bitcoin/pull/31802.
💬 maflcko commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813333039)
CI fails. Did this pass locally?
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813333039)
CI fails. Did this pass locally?
💬 fanquake commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813337803)
For reference (https://github.com/bitcoin/bitcoin/actions/runs/14517522442/job/40730094523?pr=32296#step:6:4644):
```bash
/home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:360:17: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 7287561039273463316 (64-bit, unsigned) to type 'int' changed the value to 1893666324 (32-bit, signed)
#0 0x555bd4509ce3 in secp256k1_modinv64_posdivsteps_62_var /home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h
...
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813337803)
For reference (https://github.com/bitcoin/bitcoin/actions/runs/14517522442/job/40730094523?pr=32296#step:6:4644):
```bash
/home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:360:17: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 7287561039273463316 (64-bit, unsigned) to type 'int' changed the value to 1893666324 (32-bit, signed)
#0 0x555bd4509ce3 in secp256k1_modinv64_posdivsteps_62_var /home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h
...