💬 m3dwards commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2163577539)
Updated `test_runner.py` so that:
`--exclude "rpc_bind.py --IPV6` will only exclude that variant.
`--exclude rpc_bind.py` will exclude all variants
Green test run with `TEST_RUNNER_EXTRA: --exclude "rpc_bind.py --ipv6, feature_proxy.py"`: https://github.com/m3dwards/bitcoin/actions/runs/9485948819/job/26139127116. Note that `rpc_bind.py --ipv4` and `rpc_bind.py --nonloopback` were both run but `rpc_bind.py --ipv6` and `feature_proxy.py` did not run.
Spaces between test names now also
...
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2163577539)
Updated `test_runner.py` so that:
`--exclude "rpc_bind.py --IPV6` will only exclude that variant.
`--exclude rpc_bind.py` will exclude all variants
Green test run with `TEST_RUNNER_EXTRA: --exclude "rpc_bind.py --ipv6, feature_proxy.py"`: https://github.com/m3dwards/bitcoin/actions/runs/9485948819/job/26139127116. Note that `rpc_bind.py --ipv4` and `rpc_bind.py --nonloopback` were both run but `rpc_bind.py --ipv6` and `feature_proxy.py` did not run.
Spaces between test names now also
...
👋 m3dwards's pull request is ready for review: "ci: parse TEST_RUNNER_EXTRA into an array"
(https://github.com/bitcoin/bitcoin/pull/30244)
(https://github.com/bitcoin/bitcoin/pull/30244)
💬 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...