💬 luke-jr commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2163670519)
Besides making the code cleaner, I'm hoping to get to a point where it's practical to fix the remaining vsize bugs.
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2163670519)
Besides making the code cleaner, I'm hoping to get to a point where it's practical to fix the remaining vsize bugs.
💬 furszy commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1636519093)
Structural topic (not really for this PR):
These two functions, and also `CRecipient` seem to fit better on a new `spend_util.h/cpp` file rather than here.
Also `TransactionChangeType` seem to fit better inside `spend.h/cpp` rather than here.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1636519093)
Structural topic (not really for this PR):
These two functions, and also `CRecipient` seem to fit better on a new `spend_util.h/cpp` file rather than here.
Also `TransactionChangeType` seem to fit better inside `spend.h/cpp` rather than here.
💬 furszy commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1636591875)
I must also be missing something here because you should be able to write this as:
```c++
size_t GetSerializeSizeForRecipient(const CRecipient& recipient)
{
return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(recipient.dest)));
}
```
And the follow-up PR should also compile this code:
```c++
size_t GetSerializeSizeForRecipient(const CRecipient& recipient)
{
// A Silent Payements address is instructions on how to create a WitnessV1Taproot output
...
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1636591875)
I must also be missing something here because you should be able to write this as:
```c++
size_t GetSerializeSizeForRecipient(const CRecipient& recipient)
{
return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(recipient.dest)));
}
```
And the follow-up PR should also compile this code:
```c++
size_t GetSerializeSizeForRecipient(const CRecipient& recipient)
{
// A Silent Payements address is instructions on how to create a WitnessV1Taproot output
...
💬 furszy commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1636555459)
This include should still be needed. The `CreateTransactionInternal` still calls `GetDustThreshold`.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1636555459)
This include should still be needed. The `CreateTransactionInternal` still calls `GetDustThreshold`.
💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636908381)
commit 4a7f541054d634a34e5f3cbda427e400dc1569f1:
This comment is no longer true and should be removed - the send happens below instead.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636908381)
commit 4a7f541054d634a34e5f3cbda427e400dc1569f1:
This comment is no longer true and should be removed - the send happens below instead.
💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636906205)
unrelated whitespace change?
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636906205)
unrelated whitespace change?
💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636917686)
commit 4a7f541054d634a34e5f3cbda427e400dc1569f1:
I think that this could fail intermittently (can put a sleep before this line to trigger).
I think we have to first set `can_data_be_received`, and only then send the rest of the data.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1636917686)
commit 4a7f541054d634a34e5f3cbda427e400dc1569f1:
I think that this could fail intermittently (can put a sleep before this line to trigger).
I think we have to first set `can_data_be_received`, and only then send the rest of the data.
💬 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