🤔 jonatack reviewed a pull request: "refactor: remove redundant `for` constructs"
(https://github.com/bitcoin/bitcoin/pull/31891#pullrequestreview-2621670803)
The test code is from 7488acc64685 in https://github.com/bitcoin/bitcoin/pull/27877 (cc @murchandamus). This change alters the order of adding clone coins, which doesn't appear to matter, but maybe the author had a reason for this order.
(https://github.com/bitcoin/bitcoin/pull/31891#pullrequestreview-2621670803)
The test code is from 7488acc64685 in https://github.com/bitcoin/bitcoin/pull/27877 (cc @murchandamus). This change alters the order of adding clone coins, which doesn't appear to matter, but maybe the author had a reason for this order.
💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2663787814)
> This change alters the order of adding clone coins, which doesn't appear to matter, but maybe the author had a reason for this order.
One of the first thing coin-grinder does is to sort the UTXO's by effective value, therefore, the order of the UTXO pool should not matter. Thanks for pointing this out and for taking a look.
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2663787814)
> This change alters the order of adding clone coins, which doesn't appear to matter, but maybe the author had a reason for this order.
One of the first thing coin-grinder does is to sort the UTXO's by effective value, therefore, the order of the UTXO pool should not matter. Thanks for pointing this out and for taking a look.
💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1958633307)
I'll go through this PR and check what I can add in a follow up PR. Regarding https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550 and the discussion on Linux Kernel I came to the following conclusions:
- it does not make sense to mix runtime deps. that are either built from [depends](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) or come from the system in the same table as required build deps.
- there should be a separate section for runtime deps. that **d
...
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1958633307)
I'll go through this PR and check what I can add in a follow up PR. Regarding https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550 and the discussion on Linux Kernel I came to the following conclusions:
- it does not make sense to mix runtime deps. that are either built from [depends](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) or come from the system in the same table as required build deps.
- there should be a separate section for runtime deps. that **d
...
💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2663823172)
I updated the commit message adding that the given input order does not matter since coin-grinder sorts first.
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2663823172)
I updated the commit message adding that the given input order does not matter since coin-grinder sorts first.
💬 adamandrews1 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#issuecomment-2663858278)
utACK 80e695e
(https://github.com/bitcoin/bitcoin/pull/30302#issuecomment-2663858278)
utACK 80e695e
🤔 jonatack reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2621788527)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2621788527)
Concept ACK
🤔 Sjors reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2621768049)
Also looked at the last two commits.
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2621768049)
Also looked at the last two commits.
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958677916)
In https://github.com/bitcoin/bitcoin/commit/6ec298a335585ef723d113b94959377d3ff65759 "psbt: use sighash type field to determine whether to remove non-witness utxos": any non_witness_utxos? Also we don't know if it's SegWit v0 at this point?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958677916)
In https://github.com/bitcoin/bitcoin/commit/6ec298a335585ef723d113b94959377d3ff65759 "psbt: use sighash type field to determine whether to remove non-witness utxos": any non_witness_utxos? Also we don't know if it's SegWit v0 at this point?
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958663642)
In 6ec298a335585ef723d113b94959377d3ff65759 "psbt: use sighash type field to determine whether to remove non-witness utxos":
"can only ... if not" is hard to parse, and it seems the code doing the opposite. It also seems to do the opposite of the original behavior.
```
// Only drop non_witness_utxos if sighash_type != SIGHASH_ANYONECANPAY
```
Can you clarify the reason why `SIGHASH_ANYONECANPAY` requires different treatment in the first place?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958663642)
In 6ec298a335585ef723d113b94959377d3ff65759 "psbt: use sighash type field to determine whether to remove non-witness utxos":
"can only ... if not" is hard to parse, and it seems the code doing the opposite. It also seems to do the opposite of the original behavior.
```
// Only drop non_witness_utxos if sighash_type != SIGHASH_ANYONECANPAY
```
Can you clarify the reason why `SIGHASH_ANYONECANPAY` requires different treatment in the first place?
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2663900281)
@DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2663900281)
@DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2663965111)
> It might be worth trying to notarize the binaries (instead of only the GUI bundle). This [forum thread](https://forums.developer.apple.com/forums/thread/721117) suggests it can be done by temporarily zipping them and sending them Apple in that form.
The binaries can be notarized, but the notarization cannot be stapled, which is why I elected to not notarize them. This means that anyone who runs those binaries will be phoning home to apple, but I guess that will happen regardless of notariza
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2663965111)
> It might be worth trying to notarize the binaries (instead of only the GUI bundle). This [forum thread](https://forums.developer.apple.com/forums/thread/721117) suggests it can be done by temporarily zipping them and sending them Apple in that form.
The binaries can be notarized, but the notarization cannot be stapled, which is why I elected to not notarize them. This means that anyone who runs those binaries will be phoning home to apple, but I guess that will happen regardless of notariza
...
💬 achow101 commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2663967294)
> rfm?
I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2663967294)
> rfm?
I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0
🤔 zaidmstrr reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2621909024)
Hey, I'm reviewing this PR, as the description says that we can test each target separately, (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`) which is fine, but for the combined test I can't find any fuzz target there like `coinselection`.
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2621909024)
Hey, I'm reviewing this PR, as the description says that we can test each target separately, (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`) which is fine, but for the combined test I can't find any fuzz target there like `coinselection`.
💬 Crypt-iQ commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2664024685)
I meant to reply but guess I never did. The point about blocks-only nodes is a good one. In the happy path, they aren't sent compact blocks since they never send `SENDCMPCT` to the peer again to set the high-bandwidth mode. But they can still be sent unsolicited compact blocks and because they can't reconstruct the blocks, it lets them be fingerprinted if anything is in their mempool. In most cases I don't think we can send `GETDATA` to fetch txns from blocksonly nodes since they never `INV` us
...
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2664024685)
I meant to reply but guess I never did. The point about blocks-only nodes is a good one. In the happy path, they aren't sent compact blocks since they never send `SENDCMPCT` to the peer again to set the high-bandwidth mode. But they can still be sent unsolicited compact blocks and because they can't reconstruct the blocks, it lets them be fingerprinted if anything is in their mempool. In most cases I don't think we can send `GETDATA` to fetch txns from blocksonly nodes since they never `INV` us
...
💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2664030173)
> Hey, I'm reviewing this PR, as the description says that we can test each target separately, (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`) which is fine, but for the combined test I can't find any fuzz target there like `coinselection`.
I just updated the description. I removed the `coinselection` target, so there are 3 ones.
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2664030173)
> Hey, I'm reviewing this PR, as the description says that we can test each target separately, (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`) which is fine, but for the combined test I can't find any fuzz target there like `coinselection`.
I just updated the description. I removed the `coinselection` target, so there are 3 ones.
💬 mahmoudeA commented on issue "Bitcoin Core-[test] : Text overflows during the sync process.":
(https://github.com/bitcoin-core/gui/issues/847#issuecomment-2664040590)
تم اصلاح المشكلھ
(https://github.com/bitcoin-core/gui/issues/847#issuecomment-2664040590)
تم اصلاح المشكلھ
💬 fjahr commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2664050910)
I compared the test vectors here to those in the BIP and I couldn't find the ["Spend of a Taproot output where a key in a script is a MuSig2 Aggregate Pubkey - With participant pubkeys only"](https://github.com/bitcoin/bips/pull/1764/files#diff-1cfac4ca4a777bedf8e9f21117e9a9849652e513828314234e3f7f81db1aae64R234) case here.
e.g. this Base64 String
```
cHNidP8BAFICAAAAAZqLSlB5a5YAmQ9/4R36ALxw79KWBIr8hnGa8PsfqRk3AQAAAAD9////ARjd9QUAAAAAFgAUyRI+BujX8JZsXRzQ+TMALU63V80AAAAAAAEBKwDh9QUAAAAAIlE
...
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2664050910)
I compared the test vectors here to those in the BIP and I couldn't find the ["Spend of a Taproot output where a key in a script is a MuSig2 Aggregate Pubkey - With participant pubkeys only"](https://github.com/bitcoin/bips/pull/1764/files#diff-1cfac4ca4a777bedf8e9f21117e9a9849652e513828314234e3f7f81db1aae64R234) case here.
e.g. this Base64 String
```
cHNidP8BAFICAAAAAZqLSlB5a5YAmQ9/4R36ALxw79KWBIr8hnGa8PsfqRk3AQAAAAD9////ARjd9QUAAAAAFgAUyRI+BujX8JZsXRzQ+TMALU63V80AAAAAAAEBKwDh9QUAAAAAIlE
...
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2664065518)
> Am I missing something or did this one get lost?
Looks like it got lost. Added it now.
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2664065518)
> Am I missing something or did this one get lost?
Looks like it got lost. Added it now.
💬 mlori commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2664077362)
## Benchmark Results for Bitcoin Core Optimization
Based on the guide: [https://gist.github.com/l0rinc/83d2bdfce378ad7396610095ceb7bed5](https://gist.github.com/l0rinc/83d2bdfce378ad7396610095ceb7bed5), I executed the following benchmark tests on the commits listed below, with **4 runs per commit**.
- **caa68f79c11e5c444977ce8dee8a43020b7b3c5a** (Optimization: XOR 64 bits together instead of byte-by-byte)
- **5acf12bafeb126f2190b3f401f95199e0eea90c9** (master)
I began this quest on Jan
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2664077362)
## Benchmark Results for Bitcoin Core Optimization
Based on the guide: [https://gist.github.com/l0rinc/83d2bdfce378ad7396610095ceb7bed5](https://gist.github.com/l0rinc/83d2bdfce378ad7396610095ceb7bed5), I executed the following benchmark tests on the commits listed below, with **4 runs per commit**.
- **caa68f79c11e5c444977ce8dee8a43020b7b3c5a** (Optimization: XOR 64 bits together instead of byte-by-byte)
- **5acf12bafeb126f2190b3f401f95199e0eea90c9** (master)
I began this quest on Jan
...
💬 1440000bytes commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2664080542)
Now that I look back this feels like social engineering attempt or a troll.
Anyways if @InnDe you are able to access your wallet, please let us know.
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2664080542)
Now that I look back this feels like social engineering attempt or a troll.
Anyways if @InnDe you are able to access your wallet, please let us know.