💬 brunoerg commented on pull request "fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read":
(https://github.com/bitcoin/bitcoin/pull/30273#issuecomment-2163813661)
From CI:
```
SUMMARY: UndefinedBehaviorSanitizer: invalid-null-argument test/fuzz/util/net.cpp:219:66
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x5c,0x57,0x5c,0x57,0x5c,0xfb,0xff,0x29,0xa5,0x6,0x6,0xff,0x29,0x35,0xda,
\\W\\W\\\373\377)\245\006\006\377)5\332
artifact_prefix='./'; Test unit written to ./crash-c88fdfee4792d18945ffb11a7a520d9f0fb5e56b
Base64: XFdcV1z7/ymlBgb/KTXa
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_
...
(https://github.com/bitcoin/bitcoin/pull/30273#issuecomment-2163813661)
From CI:
```
SUMMARY: UndefinedBehaviorSanitizer: invalid-null-argument test/fuzz/util/net.cpp:219:66
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x5c,0x57,0x5c,0x57,0x5c,0xfb,0xff,0x29,0xa5,0x6,0x6,0xff,0x29,0x35,0xda,
\\W\\W\\\373\377)\245\006\006\377)5\332
artifact_prefix='./'; Test unit written to ./crash-c88fdfee4792d18945ffb11a7a520d9f0fb5e56b
Base64: XFdcV1z7/ymlBgb/KTXa
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_
...
💬 furszy commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1637004599)
No need to lock the chainman mutex for `IsInitialBlockDownload()`. The function already locks it internally.
Still, I think we shouldn't use that. The more we lock `cs_main`, the more unresponsive the software is. Could use a combination of `peerman.ApproximateBestBlockDepth()` with a constant like we do inside the desirable services flags variation (`GetDesirableServiceFlags`). Or the `m_initial_sync_finished` field.
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1637004599)
No need to lock the chainman mutex for `IsInitialBlockDownload()`. The function already locks it internally.
Still, I think we shouldn't use that. The more we lock `cs_main`, the more unresponsive the software is. Could use a combination of `peerman.ApproximateBestBlockDepth()` with a constant like we do inside the desirable services flags variation (`GetDesirableServiceFlags`). Or the `m_initial_sync_finished` field.
💬 furszy commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1637037897)
Isn't this going to always reschedule the task on the first run?
Also, the active height refers to the latest connected block. It doesn't tell us we are up-to-date with the network. To know if we are sync, should use the best known header or call to the `ApproximateBestBlockDepth()` function.
And thinking more about this; what about adjusting the check interval based on the distance between the active chain height and the best header height?
I know this could vary a lot but.. something
...
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1637037897)
Isn't this going to always reschedule the task on the first run?
Also, the active height refers to the latest connected block. It doesn't tell us we are up-to-date with the network. To know if we are sync, should use the best known header or call to the `ApproximateBestBlockDepth()` function.
And thinking more about this; what about adjusting the check interval based on the distance between the active chain height and the best header height?
I know this could vary a lot but.. something
...
💬 luke-jr commented on pull request "wallet: Improve error log color in the console":
(https://github.com/bitcoin-core/gui/pull/823#issuecomment-2163842891)
Pink on white probably isn't a good idea...
(https://github.com/bitcoin-core/gui/pull/823#issuecomment-2163842891)
Pink on white probably isn't a good idea...
💬 luke-jr commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2163848450)
>To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted.
If we're preemptively making UX changes just to avoid changing UX in the future, it seems like we shouldn't take shortcuts like this... :/
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2163848450)
>To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted.
If we're preemptively making UX changes just to avoid changing UX in the future, it seems like we shouldn't take shortcuts like this... :/
💬 naiyoma commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2163875850)
> How about squashing some of these changes to help keep the master branch commit log cleaner?
Done
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2163875850)
> How about squashing some of these changes to help keep the master branch commit log cleaner?
Done
⚠️ benma opened an issue: "docs: Wrong/outdated docs for `tr(KEY)` in doc/descriptors.md"
(https://github.com/bitcoin/bitcoin/issues/30279)
Bitcoin Core uses BIP-386 for `tr(...)` descriptors, and in the case of `tr(KEY)`, the produced key is tweaked according to BIP-341:
https://github.com/bitcoin/bips/blob/85cda4e225b4d5fd7aff403f69d827f23f6afbbc/bip-0341.mediawiki?plain=1#L156
In the current descriptors.md, it is documented that `tr(KEY)` uses `KEY` as the internal key, with no further comment:
https://github.com/bitcoin/bitcoin/blob/a7bc9b76e73f04dfe4d6ba42033fe38659090e8b/doc/descriptors.md?plain=1#L82
By looking o
...
(https://github.com/bitcoin/bitcoin/issues/30279)
Bitcoin Core uses BIP-386 for `tr(...)` descriptors, and in the case of `tr(KEY)`, the produced key is tweaked according to BIP-341:
https://github.com/bitcoin/bips/blob/85cda4e225b4d5fd7aff403f69d827f23f6afbbc/bip-0341.mediawiki?plain=1#L156
In the current descriptors.md, it is documented that `tr(KEY)` uses `KEY` as the internal key, with no further comment:
https://github.com/bitcoin/bitcoin/blob/a7bc9b76e73f04dfe4d6ba42033fe38659090e8b/doc/descriptors.md?plain=1#L82
By looking o
...
💬 achow101 commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2163880342)
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2163880342)
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
🚀 achow101 merged a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015)
(https://github.com/bitcoin/bitcoin/pull/29015)
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637102449)
Done. I've encapsulated the `std::pair<SetType, FeeFrac>` for representing sets with their feerates into a new `SetInfo<SetType>` type, and changed the return type of candidate finders which count iterations to be `std::pair<SetInfo<SetType>, uint64_t>`, representing the found candidate and the number of performed iterations. I've also changed the Linearization functions to just return a `bool optimal`, rather than an iteration count (as it wasn't used anywhere anyway).
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637102449)
Done. I've encapsulated the `std::pair<SetType, FeeFrac>` for representing sets with their feerates into a new `SetInfo<SetType>` type, and changed the return type of candidate finders which count iterations to be `std::pair<SetInfo<SetType>, uint64_t>`, representing the found candidate and the number of performed iterations. I've also changed the Linearization functions to just return a `bool optimal`, rather than an iteration count (as it wasn't used anywhere anyway).
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637102621)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637102621)
Done.
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637102708)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637102708)
Done.
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637103088)
Indeed. I've added a comment. LMK whether it's clearer now.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1637103088)
Indeed. I've added a comment. LMK whether it's clearer now.
💬 achow101 commented on pull request "Lint: Support running individual lint checks":
(https://github.com/bitcoin/bitcoin/pull/30219#issuecomment-2163920227)
ACK 0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784
(https://github.com/bitcoin/bitcoin/pull/30219#issuecomment-2163920227)
ACK 0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784
🚀 achow101 merged a pull request: "Lint: Support running individual lint checks"
(https://github.com/bitcoin/bitcoin/pull/30219)
(https://github.com/bitcoin/bitcoin/pull/30219)
💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2163926995)
> > To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted.
>
> If we're preemptively making UX changes just to avoid changing UX in the future, it seems like we shouldn't take shortcuts like this... :/
We already do this in the RPC too, for exactly the same reason.
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2163926995)
> > To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted.
>
> If we're preemptively making UX changes just to avoid changing UX in the future, it seems like we shouldn't take shortcuts like this... :/
We already do this in the RPC too, for exactly the same reason.
💬 achow101 commented on issue "docs: Wrong/outdated docs for `tr(KEY)` in doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2163930686)
I thought the phrase "internal key" is unambiguous to mean that the key will need tweaking, as that is how BIP 341 defines that phrase.
It might make sense to remove descriptors.md since it's ostensibly superseded by the BIPs too.
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2163930686)
I thought the phrase "internal key" is unambiguous to mean that the key will need tweaking, as that is how BIP 341 defines that phrase.
It might make sense to remove descriptors.md since it's ostensibly superseded by the BIPs too.
💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2163943260)
Pushed 09b90c6 which makes the tests deterministic by using predictable 'random' numbers.
Using this to reproduce the issue @dergoegge pointed out:
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 695e8d806a..64d635a97a 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const {
uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
static_ass
...
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2163943260)
Pushed 09b90c6 which makes the tests deterministic by using predictable 'random' numbers.
Using this to reproduce the issue @dergoegge pointed out:
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 695e8d806a..64d635a97a 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const {
uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
static_ass
...
💬 AngusP commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1637144977)
You can `response.read(amt=<number of bytes>)` so it could be chunked to avoid loading the whole thing in to memory. That *could* also be used to add back some coarse progress indicator
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1637144977)
You can `response.read(amt=<number of bytes>)` so it could be chunked to avoid loading the whole thing in to memory. That *could* also be used to add back some coarse progress indicator
💬 AngusP commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1637147093)
With curl that makes sense, though here the `urlopen` will raise a `HTTPError` on a 404 for both a `GET` and `HEAD` request, so the earlier HEAD does seem superfluous?
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1637147093)
With curl that makes sense, though here the `urlopen` will raise a `HTTPError` on a 404 for both a `GET` and `HEAD` request, so the earlier HEAD does seem superfluous?