💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636934801)
commit 12b9aa22d6a74e4f6e9ef9672569f19d33d4f52b:
I think that the test would fail if `generate_keypair_and_garbage()` returns 0 garbage bytes, so `MAX_GARBAGE_LEN + 1` might be needed here.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636934801)
commit 12b9aa22d6a74e4f6e9ef9672569f19d33d4f52b:
I think that the test would fail if `generate_keypair_and_garbage()` returns 0 garbage bytes, so `MAX_GARBAGE_LEN + 1` might be needed here.
💬 Scamreno commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2163749506)
U want me to reply
On Wed, Jun 12, 2024, 9:28 AM maflcko ***@***.***> wrote:
> I think the issue remains that the rescan argument is boolean and optional
> in importaddress, which IIRC was one of the reasons to move toward
> importdescriptors. Yes, the importdescriptors interface is non-trivial,
> but I don't see how the burden can be taken off the user to think about the
> rescan timestamp of the descriptor. It needs to be accurate, because if it
> is too late, funds/txs may be misse
...
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2163749506)
U want me to reply
On Wed, Jun 12, 2024, 9:28 AM maflcko ***@***.***> wrote:
> I think the issue remains that the rescan argument is boolean and optional
> in importaddress, which IIRC was one of the reasons to move toward
> importdescriptors. Yes, the importdescriptors interface is non-trivial,
> but I don't see how the burden can be taken off the user to think about the
> rescan timestamp of the descriptor. It needs to be accurate, because if it
> is too late, funds/txs may be misse
...
💬 Scamreno commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2163753965)
#29772
On Wed, Jun 12, 2024, 3:25 PM Scam Reno ***@***.***> wrote:
> U want me to reply
>
> On Wed, Jun 12, 2024, 9:28 AM maflcko ***@***.***> wrote:
>
>> I think the issue remains that the rescan argument is boolean and
>> optional in importaddress, which IIRC was one of the reasons to move
>> toward importdescriptors. Yes, the importdescriptors interface is
>> non-trivial, but I don't see how the burden can be taken off the user to
>> think about the rescan timestamp of the descr
...
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2163753965)
#29772
On Wed, Jun 12, 2024, 3:25 PM Scam Reno ***@***.***> wrote:
> U want me to reply
>
> On Wed, Jun 12, 2024, 9:28 AM maflcko ***@***.***> wrote:
>
>> I think the issue remains that the rescan argument is boolean and
>> optional in importaddress, which IIRC was one of the reasons to move
>> toward importdescriptors. Yes, the importdescriptors interface is
>> non-trivial, but I don't see how the burden can be taken off the user to
>> think about the rescan timestamp of the descr
...
💬 brunoerg commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1636990489)
Yes, you're right. Since i2p is `CThreadInterrupt*`. Thanks.
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1636990489)
Yes, you're right. Since i2p is `CThreadInterrupt*`. Thanks.
💬 brunoerg commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2163773284)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1636502596
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2163773284)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1636502596
💬 luke-jr commented on pull request "Allow to configure custom libzmq prefix":
(https://github.com/bitcoin/bitcoin/pull/30256#issuecomment-2163798160)
Concept ~0. Doesn't seem worth it if we're moving to cmake soon.
(https://github.com/bitcoin/bitcoin/pull/30256#issuecomment-2163798160)
Concept ~0. Doesn't seem worth it if we're moving to cmake soon.
📝 brunoerg opened a pull request: "test: cover more errors for `signrawtransactionwithkey` RPC"
(https://github.com/bitcoin/bitcoin/pull/30278)
This PR adds test coverage for the following errors for the `signrawtransactionwithkey` RPC:
- Invalid private key
- TX decode failed
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
(https://github.com/bitcoin/bitcoin/pull/30278)
This PR adds test coverage for the following errors for the `signrawtransactionwithkey` RPC:
- Invalid private key
- TX decode failed
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
💬 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.