🚀 fanquake merged a pull request: "doc: fix `LIBRARY_PATH` comment"
(https://github.com/bitcoin/bitcoin/pull/33308)
(https://github.com/bitcoin/bitcoin/pull/33308)
👍 instagibbs approved a pull request: "net: check for empty header before calling FillBlock"
(https://github.com/bitcoin/bitcoin/pull/33296#pullrequestreview-3196566409)
ACK 2d31c5ae72940c594ec19d745bdbf09284c257d2
(https://github.com/bitcoin/bitcoin/pull/33296#pullrequestreview-3196566409)
ACK 2d31c5ae72940c594ec19d745bdbf09284c257d2
💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3266420083)
Approach ACK on using siphash and `FlatFilePos`.
Do you want to squash the commits? This just feels like an improvement in every regard.
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3266420083)
Approach ACK on using siphash and `FlatFilePos`.
Do you want to squash the commits? This just feels like an improvement in every regard.
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-3266487082)
`67bc008f62...0cd86a5c70`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-3266487082)
`67bc008f62...0cd86a5c70`: rebase due to conflicts
💬 fjahr commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330441916)
Done
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330441916)
Done
💬 fjahr commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330442142)
It doesn't modify the data but the storage on disk but since there are changes I think you are right and this shouldn't be `const`. I don't think it's `noexcept` because we are doing stuff on disk there are a lot issues that could come from that.
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330442142)
It doesn't modify the data but the storage on disk but since there are changes I think you are right and this shouldn't be `const`. I don't think it's `noexcept` because we are doing stuff on disk there are a lot issues that could come from that.
💬 fjahr commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330442363)
I am pretty indifferent, so making it debug. I had to add a new category since none of the existing seemed to fit.
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330442363)
I am pretty indifferent, so making it debug. I had to add a new category since none of the existing seemed to fit.
💬 fjahr commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330442716)
As I and others have mentioned (see the linked PR), this doesn't work for coinstatsindex and we end up with hundreds of 55kb files that seem to stay around forever or until we trigger a full compaction manually.
Benchmarking didn't seem to make sense to me because the compaction completes almost infinitely so comparing two runs with compaction and without may not have given a result that gives an accurate answer. But I figured if I time the runtime of each compaction to the millisecond and th
...
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330442716)
As I and others have mentioned (see the linked PR), this doesn't work for coinstatsindex and we end up with hundreds of 55kb files that seem to stay around forever or until we trigger a full compaction manually.
Benchmarking didn't seem to make sense to me because the compaction completes almost infinitely so comparing two runs with compaction and without may not have given a result that gives an accurate answer. But I figured if I time the runtime of each compaction to the millisecond and th
...
💬 fjahr commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330443068)
DOne
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2330443068)
DOne
💬 ryanofsky commented on issue "ipc: crash on `submitSolution` call with invalid coinbase":
(https://github.com/bitcoin/bitcoin/issues/33341#issuecomment-3266653805)
Nice find. This behavior is expected but should be improved, and shouldn't be hard to improve.
It's similar to https://github.com/bitcoin-core/libmultiprocess/issues/206 in that if a bad request is sent to the node, the node will crash instead of returning an error, when it could just as easily return an error, and it would make sense to do that for the sake of stability and to make development easier with other languages like rust and python.
Similar to https://github.com/bitcoin-core/libmult
...
(https://github.com/bitcoin/bitcoin/issues/33341#issuecomment-3266653805)
Nice find. This behavior is expected but should be improved, and shouldn't be hard to improve.
It's similar to https://github.com/bitcoin-core/libmultiprocess/issues/206 in that if a bad request is sent to the node, the node will crash instead of returning an error, when it could just as easily return an error, and it would make sense to do that for the sake of stability and to make development easier with other languages like rust and python.
Similar to https://github.com/bitcoin-core/libmult
...
🤔 fjahr reviewed a pull request: "net: Add interrupt to pcp retry loop"
(https://github.com/bitcoin/bitcoin/pull/33338#pullrequestreview-3196901800)
utACK 188de70c86414b8b2ad5143f5c607b67686526ea
Looks good to me, happy to re-review if you decide to address the nits.
(https://github.com/bitcoin/bitcoin/pull/33338#pullrequestreview-3196901800)
utACK 188de70c86414b8b2ad5143f5c607b67686526ea
Looks good to me, happy to re-review if you decide to address the nits.
💬 fjahr commented on pull request "net: Add interrupt to pcp retry loop":
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2330502356)
nit: Could add a simple test where the interrupt is instantly called to make sure the line is covered somehow.
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2330502356)
nit: Could add a simple test where the interrupt is instantly called to make sure the line is covered somehow.
💬 fjahr commented on pull request "net: Add interrupt to pcp retry loop":
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2330498571)
nit: comment above lists all the conditions for the wait to exit so it should list interrupt as well.
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2330498571)
nit: comment above lists all the conditions for the wait to exit so it should list interrupt as well.
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330027258)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Can consider using this `change_addr` below like `addr` is used to have fewer moving parts.
```diff
- psbt = wallets[0].send(outputs=[{def_wallet.getnewaddress(): 5}], inputs=[utxo], change_type="bech32m")["psbt"]
+ psbt = wallets[0].send(outputs=[{def_wallet.getnewaddress(): 5}], inputs=[utxo], change_address= change_addr)["psbt"]
```
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330027258)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Can consider using this `change_addr` below like `addr` is used to have fewer moving parts.
```diff
- psbt = wallets[0].send(outputs=[{def_wallet.getnewaddress(): 5}], inputs=[utxo], change_type="bech32m")["psbt"]
+ psbt = wallets[0].send(outputs=[{def_wallet.getnewaddress(): 5}], inputs=[utxo], change_address= change_addr)["psbt"]
```
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330014983)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Because the imported `musig` descriptor was an active one, the change address that is created here is also musig that leads to a `outputs` property in the decoded PSBT that is untested at the moment unlike the `inputs` section that is asserted on down below.
IMO we should check for the musig properties in the `output` section too.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330014983)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Because the imported `musig` descriptor was an active one, the change address that is created here is also musig that leads to a `outputs` property in the decoded PSBT that is untested at the moment unlike the `inputs` section that is asserted on down below.
IMO we should check for the musig properties in the `output` section too.
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330079926)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
For the last 4 patterns, I see the following error:
```
test_framework.authproxy.JSONRPCException: Can't get descriptor string. (-4)
```
if the private descs are listed immediately after importing musig descriptor:
```python
wallets[0].listdescriptors(True)["descriptors"]
```
The first 5 patterns work fine - I am guessing because of this default from the previous
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330079926)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
For the last 4 patterns, I see the following error:
```
test_framework.authproxy.JSONRPCException: Can't get descriptor string. (-4)
```
if the private descs are listed immediately after importing musig descriptor:
```python
wallets[0].listdescriptors(True)["descriptors"]
```
The first 5 patterns work fine - I am guessing because of this default from the previous
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2329956805)
In d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
`MULTIPATH_RE` is unused.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2329956805)
In d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
`MULTIPATH_RE` is unused.
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330109213)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Before spending, would be good to check that wallets agree on the received UTXO.
```python
# Check that the wallets agree on the received UTXO
utxo = None
for wallet in wallets:
if utxo is None:
utxo = wallet.listunspent()[0]
else:
assert_equal(utxo, wallet.listunspent()[0])
```
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330109213)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Before spending, would be good to check that wallets agree on the received UTXO.
```python
# Check that the wallets agree on the received UTXO
utxo = None
for wallet in wallets:
if utxo is None:
utxo = wallet.listunspent()[0]
else:
assert_equal(utxo, wallet.listunspent()[0])
```
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330153004)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
```diff
- # Construct and import each wallet's musig descriptor
+ # Construct and import each wallet's musig descriptor that
+ # contains private key of that wallet and public keys of others
```
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330153004)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
```diff
- # Construct and import each wallet's musig descriptor
+ # Construct and import each wallet's musig descriptor that
+ # contains private key of that wallet and public keys of others
```
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330145621)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Nit for clarity:
```diff
- exp_key_leaf = 0
+ expected_key_leaves_count = 0
```
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330145621)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Nit for clarity:
```diff
- exp_key_leaf = 0
+ expected_key_leaves_count = 0
```
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330311229)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
This is a pretty packed function and slightly difficult to follow - can consider splitting this function into 2 because it can be noticed that first portion is preparing the wallets for spending and the second portion is spending from the musig address while asserting on the relevant data.
<details open>
<summary>Function Splitting Diff</summary>
```diff
diff --git a
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330311229)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
This is a pretty packed function and slightly difficult to follow - can consider splitting this function into 2 because it can be noticed that first portion is preparing the wallets for spending and the second portion is spending from the musig address while asserting on the relevant data.
<details open>
<summary>Function Splitting Diff</summary>
```diff
diff --git a
...