đ stickies-v approved a pull request: "test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585)"
(https://github.com/bitcoin/bitcoin/pull/28725#pullrequestreview-1735251356)
ACK a478c817b2f62b7334b36e331a2e37fe8380c754
(https://github.com/bitcoin/bitcoin/pull/28725#pullrequestreview-1735251356)
ACK a478c817b2f62b7334b36e331a2e37fe8380c754
đŦ jonasnick commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815234541)
@achow101 @seedhammer
> you will lose the information about the pubkeys that are involved in that aggregated pubkey, and that information is necessary for spending. It will still need to be stored and backed up somewhere.
As @AdamISZ notes above it would be sufficient to store the aggregate pubkey, let some third party provide the individual pubkeys and then check whether the aggregate of the individual pubkeys matches the stored aggregate pubkey. Due to the collision resistance property o
...
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815234541)
@achow101 @seedhammer
> you will lose the information about the pubkeys that are involved in that aggregated pubkey, and that information is necessary for spending. It will still need to be stored and backed up somewhere.
As @AdamISZ notes above it would be sufficient to store the aggregate pubkey, let some third party provide the individual pubkeys and then check whether the aggregate of the individual pubkeys matches the stored aggregate pubkey. Due to the collision resistance property o
...
đŦ sipa commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815241984)
I'm a bit confused by the discussion here.
The point of descriptors is encoding all information necessary to spend funds (apart from private key material, which is optional). If you remove information like which the co-signer public keys are, then signing no longer works. Of course, you *could* get that information from your cosigners, but if that's acceptable, why can't you get the descriptor in its entirety from the cosigners (and if desired, just keep the (master) private key)?
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815241984)
I'm a bit confused by the discussion here.
The point of descriptors is encoding all information necessary to spend funds (apart from private key material, which is optional). If you remove information like which the co-signer public keys are, then signing no longer works. Of course, you *could* get that information from your cosigners, but if that's acceptable, why can't you get the descriptor in its entirety from the cosigners (and if desired, just keep the (master) private key)?
đŦ jonasnick commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815255072)
@sipa
> Of course, you *could* get that information from your cosigners, but if that's acceptable, why can't you get the descriptor in its entirety from the cosigners (and if desired, just keep the (master) private key)?
My understanding was that it still makes sense to backup the descriptor even though its not sufficient to sign because it commits to your set of co-signers. If you don't back up such a descriptor and instead obtain it from somewhere, then you have to verify again somehow
...
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815255072)
@sipa
> Of course, you *could* get that information from your cosigners, but if that's acceptable, why can't you get the descriptor in its entirety from the cosigners (and if desired, just keep the (master) private key)?
My understanding was that it still makes sense to backup the descriptor even though its not sufficient to sign because it commits to your set of co-signers. If you don't back up such a descriptor and instead obtain it from somewhere, then you have to verify again somehow
...
đŦ sipa commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815259028)
> My understanding was that it still makes sense to backup the descriptor even though its not sufficient to sign because it commits to your set of co-signers.
The address also commits to the set of co-signers.
But see my edit above: if you're going to do aggregation and then BIP32 derivation on top, it makes sense to backup the descriptor with the aggregation pre-evaluated; that's effectively equivalent to having the information for all addresses you'd want to derive with it.
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1815259028)
> My understanding was that it still makes sense to backup the descriptor even though its not sufficient to sign because it commits to your set of co-signers.
The address also commits to the set of co-signers.
But see my edit above: if you're going to do aggregation and then BIP32 derivation on top, it makes sense to backup the descriptor with the aggregation pre-evaluated; that's effectively equivalent to having the information for all addresses you'd want to derive with it.
đŦ mzumsande commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815290138)
> Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.
I think I disagree with that. They might now be protected from an eviction, but that eviction is also new: Before, we only had the stale-tip eviction, and I think that would only extremely rarely evict such a peer.
I think that the mechanism mentioned in the OP (race after getting disconnected) is probably what makes this more noticeable, becaus `Threa
...
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815290138)
> Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.
I think I disagree with that. They might now be protected from an eviction, but that eviction is also new: Before, we only had the stale-tip eviction, and I think that would only extremely rarely evict such a peer.
I think that the mechanism mentioned in the OP (race after getting disconnected) is probably what makes this more noticeable, becaus `Threa
...
đŦ KST-Energy commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1815303802)
Hi, found this issue while looking for a fix, and want to share my experience.
```
$ bitcoin-qt
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
```
I have essentially installed core (24.2) using the instructions here : [](https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#dependency-build-instructions) with all of these dependencies:
`sudo apt-get install libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-d
...
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1815303802)
Hi, found this issue while looking for a fix, and want to share my experience.
```
$ bitcoin-qt
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
```
I have essentially installed core (24.2) using the instructions here : [](https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#dependency-build-instructions) with all of these dependencies:
`sudo apt-get install libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-d
...
đŦ mzumsande commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815306349)
Tested ACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1
I tested this by running on cjdns and ipv4, and then manually disconnecting an addnode cjdns peer and observing the log - both on v26.0rc2 and this branch.
> Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.
I think I disagree with that. They might now be protected from network-specific eviction, but that eviction is also new: Before #27213, we only h
...
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815306349)
Tested ACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1
I tested this by running on cjdns and ipv4, and then manually disconnecting an addnode cjdns peer and observing the log - both on v26.0rc2 and this branch.
> Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.
I think I disagree with that. They might now be protected from network-specific eviction, but that eviction is also new: Before #27213, we only h
...
đŦ petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1815321667)
On Thu, Nov 16, 2023 at 12:25:20AM -0800, LÊo Haf wrote:
> Congratulations, what some feared finally [happened](https://x.com/mononautical/status/1724943620888006802?s=20) without us doing anything.
>
> @Glozow luke added the tests weeks ago, it is more than time to review this PR, you are in charge of the mempool and it is in a disastrous state with no less than 200,000 spam transactions.
The tweet you link shows how pointless this pull-req is.
ViaBTC is actively mining BRC-20 transactions,
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1815321667)
On Thu, Nov 16, 2023 at 12:25:20AM -0800, LÊo Haf wrote:
> Congratulations, what some feared finally [happened](https://x.com/mononautical/status/1724943620888006802?s=20) without us doing anything.
>
> @Glozow luke added the tests weeks ago, it is more than time to review this PR, you are in charge of the mempool and it is in a disastrous state with no less than 200,000 spam transactions.
The tweet you link shows how pointless this pull-req is.
ViaBTC is actively mining BRC-20 transactions,
...
đŦ jonatack commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815327046)
> A completely unrelated problem is that this problem exists because cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I'm not sure what to do about that.
I agree. When one-click I2P support was added to Raspiblitz and Umbrel in December 2022 after [this workshop and blog post](https://jonatack.github.io/articles/using-alternative-p2p-networks-with-bitcoin-core), the number of recent peers returned by -addrinfo for me went from
...
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815327046)
> A completely unrelated problem is that this problem exists because cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I'm not sure what to do about that.
I agree. When one-click I2P support was added to Raspiblitz and Umbrel in December 2022 after [this workshop and blog post](https://jonatack.github.io/articles/using-alternative-p2p-networks-with-bitcoin-core), the number of recent peers returned by -addrinfo for me went from
...
đŦ jonatack commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#discussion_r1396364317)
Note to self when retouching here or in follow-ups (i.e. #28248)
```suggestion
for (const auto& node : connman->TestNodes()) {
```
(https://github.com/bitcoin/bitcoin/pull/28895#discussion_r1396364317)
Note to self when retouching here or in follow-ups (i.e. #28248)
```suggestion
for (const auto& node : connman->TestNodes()) {
```
đŦ TheCharlatan commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1815398183)
Re https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954
> However, now that things are moving toward references, which are easy to copy accidentally. It may lead to bugs such as the one mentioned in the boost docs:
Thanks for highlighting this.
> So disallowing copies would be a good belt-and-suspenders?
I did a quick POC where `mapTx.insert` is replaced with `mapTx.emplace_back` and the `CTxMemPoolEntry` constructor arguments are passed to `emplace_back`. Then I co
...
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1815398183)
Re https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954
> However, now that things are moving toward references, which are easy to copy accidentally. It may lead to bugs such as the one mentioned in the boost docs:
Thanks for highlighting this.
> So disallowing copies would be a good belt-and-suspenders?
I did a quick POC where `mapTx.insert` is replaced with `mapTx.emplace_back` and the `CTxMemPoolEntry` constructor arguments are passed to `emplace_back`. Then I co
...
đ BrandonOdiwuor approved a pull request: "doc: Add example of mixing private and public keys in descriptors"
(https://github.com/bitcoin/bitcoin/pull/28373#pullrequestreview-1735814968)
Re ACK d3f24f5bd0a84bdd893a351e7987a388a84324f3
Confirmed change since last review is the descriptor sh(multi(2,xprv.../84'/0'/0'/0/0,xpub1...,xpub2...))
(https://github.com/bitcoin/bitcoin/pull/28373#pullrequestreview-1735814968)
Re ACK d3f24f5bd0a84bdd893a351e7987a388a84324f3
Confirmed change since last review is the descriptor sh(multi(2,xprv.../84'/0'/0'/0/0,xpub1...,xpub2...))
đŦ D33r-Gee commented on issue "26.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1815595137)
> @D33r-Gee one of the pieces of feedback I received was to differentiate between when it was asking the user to enter a command and when it was showing the result of a command. It was suggested to use $ to make the distinction.
>
> Noted your comment and will see what others think.
Make sense on the distinction it is definitely a nit, just got used to previous guides (i.e. [25.0 rc](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide)) If others find
...
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1815595137)
> @D33r-Gee one of the pieces of feedback I received was to differentiate between when it was asking the user to enter a command and when it was showing the result of a command. It was suggested to use $ to make the distinction.
>
> Noted your comment and will see what others think.
Make sense on the distinction it is definitely a nit, just got used to previous guides (i.e. [25.0 rc](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide)) If others find
...
đŦ ishaanam commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1396578864)
This is done for every pre-selected input, even the ones that don't specify the sequence. This seems confusing and also means that a transaction with any pre-selected inputs won't do any anti-fee-sniping.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1396578864)
This is done for every pre-selected input, even the ones that don't specify the sequence. This seems confusing and also means that a transaction with any pre-selected inputs won't do any anti-fee-sniping.
đ¤ ishaanam reviewed a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1735859998)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1735859998)
Concept ACK
đŦ ajtowns commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396618046)
A running tally doesn't help -- we need to write the size of the serialized data before we serialize it, so this would mean caching the serialization until we've finished first which would likely be a bigger waste of memory than running things through the size computation first is a waste of cpu. (I've looked at this at least twice with exactly the same idea already :smile:)
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396618046)
A running tally doesn't help -- we need to write the size of the serialized data before we serialize it, so this would mean caching the serialization until we've finished first which would likely be a bigger waste of memory than running things through the size computation first is a waste of cpu. (I've looked at this at least twice with exactly the same idea already :smile:)
đŦ ajtowns commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396620277)
`explicit` is good for constructors that take a single argument to prevent accidental conversion, but seems useless for constructors that don't take an argument. (The real question is why leave a constructor there at all?)
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396620277)
`explicit` is good for constructors that take a single argument to prevent accidental conversion, but seems useless for constructors that don't take an argument. (The real question is why leave a constructor there at all?)
đŦ ajtowns commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396620430)
(leaving the nits for if it's otherwise necessary to retouch)
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396620430)
(leaving the nits for if it's otherwise necessary to retouch)
đŦ ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1396624902)
Yeah. This just means changing the final `catch` to `return BracketStr(HexStr(vch));`, no?
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1396624902)
Yeah. This just means changing the final `catch` to `return BracketStr(HexStr(vch));`, no?