🚀 achow101 merged a pull request: "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256"
(https://github.com/bitcoin/bitcoin/pull/29180)
(https://github.com/bitcoin/bitcoin/pull/29180)
💬 theStack commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1468264402)
> FillableSigningProvider is just the legacy class and contains legacy scripts limitations (thus https://github.com/bitcoin/bitcoin/pull/28307). It does not affect the current form of this PR but, because this benchmark uses segwit v0 and v1 outputs, it could cause issues in the future.
Ok, good to know.
> I never said that it was going to make derivation easier. I Just said that it will also save you an extra GetScriptForDestination call per script.
Took me a while now to understand wh
...
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1468264402)
> FillableSigningProvider is just the legacy class and contains legacy scripts limitations (thus https://github.com/bitcoin/bitcoin/pull/28307). It does not affect the current form of this PR but, because this benchmark uses segwit v0 and v1 outputs, it could cause issues in the future.
Ok, good to know.
> I never said that it was going to make derivation easier. I Just said that it will also save you an extra GetScriptForDestination call per script.
Took me a while now to understand wh
...
💬 shaavan commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913035371)
Thanks, @vostrnad, for pointing out the potential flaws in my review.
Yes, I did use ChatGPT to structure my thoughts on the “problems with P2PK” in a coherent fashion, but I don’t believe the points are wrong.
Taking references from, Programming Bitcoin, Jimmy Song
https://github.com/jimmysong/programmingbitcoin/blob/master/ch06.asciidoc#problems-with-p2pk
1. Problem of Security: P2PK, exposes the bare public key to the world, and if ever ECDSA gets broken, we might lose our funds
...
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913035371)
Thanks, @vostrnad, for pointing out the potential flaws in my review.
Yes, I did use ChatGPT to structure my thoughts on the “problems with P2PK” in a coherent fashion, but I don’t believe the points are wrong.
Taking references from, Programming Bitcoin, Jimmy Song
https://github.com/jimmysong/programmingbitcoin/blob/master/ch06.asciidoc#problems-with-p2pk
1. Problem of Security: P2PK, exposes the bare public key to the world, and if ever ECDSA gets broken, we might lose our funds
...
💬 vostrnad commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913056465)
@shaavan
1. Putting bare public keys into the output doesn't make things materially worse if ECDSA/ECDLP gets broken. This was agreed on when designing Taproot, which is why P2TR outputs also use bare public keys (see [BIP341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-2)).
2. Yes, P2PKH has a shorter output script, but overall uses more blockspace than P2PK. It does take less space in the UTXO set, but P2PK only takes one more byte than e.g. P2TR or P2WSH. Note
...
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913056465)
@shaavan
1. Putting bare public keys into the output doesn't make things materially worse if ECDSA/ECDLP gets broken. This was agreed on when designing Taproot, which is why P2TR outputs also use bare public keys (see [BIP341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-2)).
2. Yes, P2PKH has a shorter output script, but overall uses more blockspace than P2PK. It does take less space in the UTXO set, but P2PK only takes one more byte than e.g. P2TR or P2WSH. Note
...
💬 shaavan commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913088852)
Thanks, @vostrnad, for helping me bring clarity to my understanding.
I understand that my points were made based on my limited knowledge surrounding the discussions around P2TR. And also, I made an incorrect point in my explanation.
I have struck the “Problems of P2PK” section to avoid confusion for future reviewers.
Lastly, though I might sometimes use ChatGPT to get better clarity on concepts or coherently express my thoughts, I will make sure to properly fact-check it before using an
...
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913088852)
Thanks, @vostrnad, for helping me bring clarity to my understanding.
I understand that my points were made based on my limited knowledge surrounding the discussions around P2TR. And also, I made an incorrect point in my explanation.
I have struck the “Problems of P2PK” section to avoid confusion for future reviewers.
Lastly, though I might sometimes use ChatGPT to get better clarity on concepts or coherently express my thoughts, I will make sure to properly fact-check it before using an
...
💬 ArmchairCryptologist commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1913096657)
> Hardcoded one-size-fits-all configuration is suboptimal, especially in context of mempools which are always unique (by definition).
If you do not run a mining node, you do in fact want your mempool configuration to be as compatible with the miner majority as possible if you want it to be "optimal", both from the perspective of your node and from the network as a whole. In most cases, this means using the defaults, but possibly with a larger size.
To see why, let's assume that you configu
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1913096657)
> Hardcoded one-size-fits-all configuration is suboptimal, especially in context of mempools which are always unique (by definition).
If you do not run a mining node, you do in fact want your mempool configuration to be as compatible with the miner majority as possible if you want it to be "optimal", both from the perspective of your node and from the network as a whole. In most cases, this means using the defaults, but possibly with a larger size.
To see why, let's assume that you configu
...
💬 ajtowns commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1913161408)
> Could you clarify what deprecation will actually mean for 27.0, and then the future steps?
I would have thought that for 27.x you'd just add the deprecation note, and otherwise not change any code? I assumed the code changes here were just to help make commenting easier, not necessarily intended for 27.0...
> For 27.0 specifically, I think this needs a release note. Release notes at least have some expectation of being read regularly, so a deprecation notice in there would be beneficial
...
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1913161408)
> Could you clarify what deprecation will actually mean for 27.0, and then the future steps?
I would have thought that for 27.x you'd just add the deprecation note, and otherwise not change any code? I assumed the code changes here were just to help make commenting easier, not necessarily intended for 27.0...
> For 27.0 specifically, I think this needs a release note. Release notes at least have some expectation of being read regularly, so a deprecation notice in there would be beneficial
...
💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1468498323)
@mzumsande, @sr-gi, I played with @sr-gi's idea on [this branch](https://github.com/stratospher/bitcoin/commit/276b95d766cd11d65d563805d52beace8dd32744) and wondering about 2 things:
1. can deadlocks happen? i noticed places in the test where `_send_lock` was acquired and then `p2p_lock` (and deadlocks didn't happen). am i missing something?
1. MainThread acquires `_send_lock`
2. NetworkThread acquires `p2p_lock`
3. NetworkThread would wait for MainThread to release `_send_lock`
4. Ma
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1468498323)
@mzumsande, @sr-gi, I played with @sr-gi's idea on [this branch](https://github.com/stratospher/bitcoin/commit/276b95d766cd11d65d563805d52beace8dd32744) and wondering about 2 things:
1. can deadlocks happen? i noticed places in the test where `_send_lock` was acquired and then `p2p_lock` (and deadlocks didn't happen). am i missing something?
1. MainThread acquires `_send_lock`
2. NetworkThread acquires `p2p_lock`
3. NetworkThread would wait for MainThread to release `_send_lock`
4. Ma
...
💬 dooglus commented on issue "New crash in v26.0":
(https://github.com/bitcoin-core/gui/issues/785#issuecomment-1913182621)
@mzumsande wrote:
> Looks like the crash happens within libQt5Core.so.5, for which there are no debug symbols available.
OK. I will install Qt5 packages with debug symbols and wait for it to crash again.
> I tried for a while to reproduce the crash but didn't manage. Is there something special or "unusual" about your setup, for example are you loading a large number of wallets, or are some of the wallets very large?
No, I don't think so. I only load 3 or 4 wallets on startup. Yesterd
...
(https://github.com/bitcoin-core/gui/issues/785#issuecomment-1913182621)
@mzumsande wrote:
> Looks like the crash happens within libQt5Core.so.5, for which there are no debug symbols available.
OK. I will install Qt5 packages with debug symbols and wait for it to crash again.
> I tried for a while to reproduce the crash but didn't manage. Is there something special or "unusual" about your setup, for example are you loading a large number of wallets, or are some of the wallets very large?
No, I don't think so. I only load 3 or 4 wallets on startup. Yesterd
...
💬 1440000bytes commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#discussion_r1468503986)
Concept ACK because default policy is unchanged.
> I don't care if this gets merged, personally **I don't see any benefit of having this option** (or -permitbaremultisig) and I would object to changing the default policy in this regard (similar to my objections to https://github.com/bitcoin/bitcoin/pull/28217).
It might be useful for testing and a few other use cases.
> Concept ACK, it's great to give users the option of filtering out potentially abusive txs
The only potential abuse
...
(https://github.com/bitcoin/bitcoin/pull/29309#discussion_r1468503986)
Concept ACK because default policy is unchanged.
> I don't care if this gets merged, personally **I don't see any benefit of having this option** (or -permitbaremultisig) and I would object to changing the default policy in this regard (similar to my objections to https://github.com/bitcoin/bitcoin/pull/28217).
It might be useful for testing and a few other use cases.
> Concept ACK, it's great to give users the option of filtering out potentially abusive txs
The only potential abuse
...
👋 russeree's pull request is ready for review: "redeclare nChainTx to use uint64_t"
(https://github.com/bitcoin/bitcoin/pull/29331)
(https://github.com/bitcoin/bitcoin/pull/29331)
💬 russeree commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1913242734)
Would this be a hard fork because it would be loosening the conditions for validation?
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1913242734)
Would this be a hard fork because it would be loosening the conditions for validation?
💬 luke-jr commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274)
This still isn't a wallet option...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274)
This still isn't a wallet option...
📝 hebasto opened a pull request: "Add missed `config/bitcoin-config.h` header"
(https://github.com/bitcoin/bitcoin/pull/29333)
The `config/bitcoin-config.h` header is required to provide definitions for the `USE_BDB` and `USE_SQLITE` macros.
While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
(https://github.com/bitcoin/bitcoin/pull/29333)
The `config/bitcoin-config.h` header is required to provide definitions for the `USE_BDB` and `USE_SQLITE` macros.
While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
💬 furszy commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468552774)
Isn't `bitcoin-config.h` been indirectly included by this `system.h` include? (which btw, doesn't seems to be needed).
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468552774)
Isn't `bitcoin-config.h` been indirectly included by this `system.h` include? (which btw, doesn't seems to be needed).
💬 hebasto commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468556096)
> Isn't `bitcoin-config.h` indirectly included by this `system.h` include?
It is.
> While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468556096)
> Isn't `bitcoin-config.h` indirectly included by this `system.h` include?
It is.
> While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
💬 furszy commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468568723)
it would be good to remove the unused include within this change. walletdb.cpp does not call to any system.h` function.
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468568723)
it would be good to remove the unused include within this change. walletdb.cpp does not call to any system.h` function.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539058)
In commit "coinselection: Add CoinGrinder algorithm"
Perhaps add a one-line comment for each of the 8 variables here.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539058)
In commit "coinselection: Add CoinGrinder algorithm"
Perhaps add a one-line comment for each of the 8 variables here.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513141)
In commit "coinselection: Add CoinGrinder algorithm"
Nit: non-positive effective value (0 is not allowed either, apparently)
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513141)
In commit "coinselection: Add CoinGrinder algorithm"
Nit: non-positive effective value (0 is not allowed either, apparently)
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539208)
In commit "coinselection: Add CoinGrinder algorithm"
"state transactions" -> "state transitions"?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539208)
In commit "coinselection: Add CoinGrinder algorithm"
"state transactions" -> "state transitions"?