📝 hebasto opened a pull request: "depends: Include `config.guess` and `config.sub` into `meta_depends`"
(https://github.com/bitcoin/bitcoin/pull/28870)
(https://github.com/bitcoin/bitcoin/pull/28870)
💬 willcl-ark commented on pull request "GUI getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1810068057)
Cool, that's pretty nice!
Just thinking out loud, but it does make me wonder if perhaps a new `Verify` or `Utils` button, in line with "Send | Recieve | Transactions" could be even nicer? If we added it there, we might consider adding a few more utils to the GUI like "Decode raw transaction", "Decode Script", "Get Descriptor Info", "Verify Message" etc. which folks who use the gui generally use other software for today...
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1810068057)
Cool, that's pretty nice!
Just thinking out loud, but it does make me wonder if perhaps a new `Verify` or `Utils` button, in line with "Send | Recieve | Transactions" could be even nicer? If we added it there, we might consider adding a few more utils to the GUI like "Decode raw transaction", "Decode Script", "Get Descriptor Info", "Verify Message" etc. which folks who use the gui generally use other software for today...
💬 brunoerg commented on pull request "test: add stress test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1810074189)
> And if this is not being run as part of the functional test suite (which I only understood just now), then I am not sure it belongs there. Maybe there is precedent for something similar but to me it feels more like this is something comparable to test_utxo_snapshots.sh?
I agree. I'm inclined to move it to `contrib` instead of being a functional test. This way we could be more free to test stuff without having to be caution with CI.
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1810074189)
> And if this is not being run as part of the functional test suite (which I only understood just now), then I am not sure it belongs there. Maybe there is precedent for something similar but to me it feels more like this is something comparable to test_utxo_snapshots.sh?
I agree. I'm inclined to move it to `contrib` instead of being a functional test. This way we could be more free to test stuff without having to be caution with CI.
💬 fanquake commented on pull request "test: add stress test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1810083470)
> The idea of this test is "stressing" the addrman using asmap.
Can you clarify what this meant to do? What is it stressing, where do the magic numbers "2000, 1/3, 2/3" come from, what is expected to fail under this "stress"?
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1810083470)
> The idea of this test is "stressing" the addrman using asmap.
Can you clarify what this meant to do? What is it stressing, where do the magic numbers "2000, 1/3, 2/3" come from, what is expected to fail under this "stress"?
💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1392492963)
The `CreateTransaction()` and `FundTransaction()` want the `CWallet` parameter to be non-const. I'm struggling to find a work around but I'm trying.
Just wanted to clear this, I think the `CWallet` instance inside these functions doesn't seem to be directly mutated or changed anytime. So, do we really want to make it to const. Am I understanding this correct?
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1392492963)
The `CreateTransaction()` and `FundTransaction()` want the `CWallet` parameter to be non-const. I'm struggling to find a work around but I'm trying.
Just wanted to clear this, I think the `CWallet` instance inside these functions doesn't seem to be directly mutated or changed anytime. So, do we really want to make it to const. Am I understanding this correct?
💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1392364899)
I made an attempt at dropping `-limitdescendantsize` and friends: https://github.com/Sjors/bitcoin/tree/2022/11/cluster-mempool
I (naively) replaced ancestor and descendent limits in coin selection with the new cluster limit. At least the tests pass.
When we drop these settings anyone who uses them will get an error when starting the node. That's probably a good thing, since they should read about this change.
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1392364899)
I made an attempt at dropping `-limitdescendantsize` and friends: https://github.com/Sjors/bitcoin/tree/2022/11/cluster-mempool
I (naively) replaced ancestor and descendent limits in coin selection with the new cluster limit. At least the tests pass.
When we drop these settings anyone who uses them will get an error when starting the node. That's probably a good thing, since they should read about this change.
💬 dergoegge commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1392514354)
If `CreateTransaction()` and `FundTransaction()` do not mutate the wallet, then they should probably take a const reference instead. But glancing at `FundTransaction`, it calls `CWallet::LockCoin` which is not a const method and could probably lead to non-determinism since it appears to be modifying internal state of a wallet.
https://github.com/bitcoin/bitcoin/blob/fb85bb277670aad28fef51b7313d4a96cdaa760f/src/wallet/spend.cpp#L1388
> Just wanted to clear this, I think the CWallet instance
...
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1392514354)
If `CreateTransaction()` and `FundTransaction()` do not mutate the wallet, then they should probably take a const reference instead. But glancing at `FundTransaction`, it calls `CWallet::LockCoin` which is not a const method and could probably lead to non-determinism since it appears to be modifying internal state of a wallet.
https://github.com/bitcoin/bitcoin/blob/fb85bb277670aad28fef51b7313d4a96cdaa760f/src/wallet/spend.cpp#L1388
> Just wanted to clear this, I think the CWallet instance
...
⚠️ hebasto opened an issue: "An overflow in `TapSatisfier::FromPKHBytes`?"
(https://github.com/bitcoin/bitcoin/issues/28871)
In the master branch @ fb85bb277670aad28fef51b7313d4a96cdaa760f, the following code in the `DecodeScript` function: https://github.com/bitcoin/bitcoin/blob/fb85bb277670aad28fef51b7313d4a96cdaa760f/src/script/miniscript.h#L2312-L2313 explicitly passes 33 bytes to the `TapSatisfier::FromPKHBytes` function, which expects 32 bytes only: https://github.com/bitcoin/bitcoin/blob/fb85bb277670aad28fef51b7313d4a96cdaa760f/src/script/sign.cpp#L295-L302
(https://github.com/bitcoin/bitcoin/issues/28871)
In the master branch @ fb85bb277670aad28fef51b7313d4a96cdaa760f, the following code in the `DecodeScript` function: https://github.com/bitcoin/bitcoin/blob/fb85bb277670aad28fef51b7313d4a96cdaa760f/src/script/miniscript.h#L2312-L2313 explicitly passes 33 bytes to the `TapSatisfier::FromPKHBytes` function, which expects 32 bytes only: https://github.com/bitcoin/bitcoin/blob/fb85bb277670aad28fef51b7313d4a96cdaa760f/src/script/sign.cpp#L295-L302
💬 sipa commented on issue "An overflow in `TapSatisfier::FromPKHBytes`?":
(https://github.com/bitcoin/bitcoin/issues/28871#issuecomment-1810116173)
Not an issue; if this runs in tapscript context, the code will bail out before reaching this line.
(https://github.com/bitcoin/bitcoin/issues/28871#issuecomment-1810116173)
Not an issue; if this runs in tapscript context, the code will bail out before reaching this line.
💬 hebasto commented on issue "An overflow in `TapSatisfier::FromPKHBytes`?":
(https://github.com/bitcoin/bitcoin/issues/28871#issuecomment-1810123165)
Right. But a compiler might be not aware of that:
```
/usr/include/c++/11/bits/stl_algobase.h:431: warning: '__builtin_memcpy' writing 33 bytes into a region of size 32 overflows the destination [-Wstringop-overflow=]
431 | __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
|
/usr/include/c++/11/bits/stl_algobase.h: In function 'DecodeScript':
script/sign.cpp:299:21: note: destination object 'pubkey' of size 32
299 | XOnlyPubKey pubkey;
|
...
(https://github.com/bitcoin/bitcoin/issues/28871#issuecomment-1810123165)
Right. But a compiler might be not aware of that:
```
/usr/include/c++/11/bits/stl_algobase.h:431: warning: '__builtin_memcpy' writing 33 bytes into a region of size 32 overflows the destination [-Wstringop-overflow=]
431 | __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
|
/usr/include/c++/11/bits/stl_algobase.h: In function 'DecodeScript':
script/sign.cpp:299:21: note: destination object 'pubkey' of size 32
299 | XOnlyPubKey pubkey;
|
...
🤔 vasild reviewed a pull request: "net: improves addnode / m_added_nodes logic"
(https://github.com/bitcoin/bitcoin/pull/28155#pullrequestreview-1729644861)
ACK 0420f99f429ce2382057e101859067f40de47be0
(https://github.com/bitcoin/bitcoin/pull/28155#pullrequestreview-1729644861)
ACK 0420f99f429ce2382057e101859067f40de47be0
💬 vasild commented on pull request "net: improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1392528755)
nit, naming: maybe a different name would have been better than `ConnectNodePublic()`, given that it contains extra logic on top of the private `ConnectNode()`.
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1392528755)
nit, naming: maybe a different name would have been better than `ConnectNodePublic()`, given that it contains extra logic on top of the private `ConnectNode()`.
💬 m3dwards commented on issue "26.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1810142470)
@willcl-ark thank you for your review. Addressed all items.
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1810142470)
@willcl-ark thank you for your review. Addressed all items.
💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1810204432)
Do we actually want to resolve all forms of ambiguity? E.g. what about the distinction between an OP_1 vs. a direct push of 0x01, vs. OP_PUSHDATA1 of 0x01? I think it'd be nice if there was a one-to-one correspondence between the disassembly and the actual script bytes.
My contribution to this bikeshedding would be:
* `OP_n` -> `n` (so `-1`, `0`, `1`, ..., `10`, ..., `16`).
* Drop "OP_" prefix for other opcodes too; they're redundant.
* Other *minimal* pushes (so direct pushes, as well as
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1810204432)
Do we actually want to resolve all forms of ambiguity? E.g. what about the distinction between an OP_1 vs. a direct push of 0x01, vs. OP_PUSHDATA1 of 0x01? I think it'd be nice if there was a one-to-one correspondence between the disassembly and the actual script bytes.
My contribution to this bikeshedding would be:
* `OP_n` -> `n` (so `-1`, `0`, `1`, ..., `10`, ..., `16`).
* Drop "OP_" prefix for other opcodes too; they're redundant.
* Other *minimal* pushes (so direct pushes, as well as
...
💬 seedhammer commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1810236476)
> Beyond that, there are indeed a number of things that need to be decided:
>
> * How to integrate into descriptors. One possibility is to only support an `agg` fragment as leaf key (e.g. `agg(xpub1/.../*,xpub2/.../*)`), but it's also possible I think to support treating `agg` of xpubs as a new "xpub" itself (e.g. `agg(xpub1/...,xpub2/...)/*`) for example by defining the aggregated chaincode as a hash of the child chaincodes. This complicates signing further, however, though I don't think
...
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1810236476)
> Beyond that, there are indeed a number of things that need to be decided:
>
> * How to integrate into descriptors. One possibility is to only support an `agg` fragment as leaf key (e.g. `agg(xpub1/.../*,xpub2/.../*)`), but it's also possible I think to support treating `agg` of xpubs as a new "xpub" itself (e.g. `agg(xpub1/...,xpub2/...)/*`) for example by defining the aggregated chaincode as a hash of the child chaincodes. This complicates signing further, however, though I don't think
...
💬 fanquake commented on issue "An overflow in `TapSatisfier::FromPKHBytes`?":
(https://github.com/bitcoin/bitcoin/issues/28871#issuecomment-1810258979)
I'm assuming this is with GCC 11 under LTO? I'd suggest using a newer, "smarter" compiler. The same warnings don't appear under GCC 13.
(https://github.com/bitcoin/bitcoin/issues/28871#issuecomment-1810258979)
I'm assuming this is with GCC 11 under LTO? I'd suggest using a newer, "smarter" compiler. The same warnings don't appear under GCC 13.
💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1810260619)
re-ACK a0c254c13a3ef21e257cca3493446c632b636b15 🐜
<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 a0c254c13a3ef21e257c
...
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1810260619)
re-ACK a0c254c13a3ef21e257cca3493446c632b636b15 🐜
<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 a0c254c13a3ef21e257c
...
🤔 Sjors reviewed a pull request: "[WIP] Cluster mempool implementation"
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-1729707068)
Couple more comments / questions. To be continued...
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-1729707068)
Couple more comments / questions. To be continued...
💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1392567718)
54f39ca8f101483f5f82707689ca49431d4091e5: what if the deleted transaction makes it so there are now two clusters? This is also safe to ignore?
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1392567718)
54f39ca8f101483f5f82707689ca49431d4091e5: what if the deleted transaction makes it so there are now two clusters? This is also safe to ignore?
💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1392646700)
54f39ca8f101483f5f82707689ca49431d4091e5: shouldn't this be `fee / size`? Or do you mean to use an inverse fee rate for performance (`inv_fee_rate`)?
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1392646700)
54f39ca8f101483f5f82707689ca49431d4091e5: shouldn't this be `fee / size`? Or do you mean to use an inverse fee rate for performance (`inv_fee_rate`)?