💬 jarolrod commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1621182702)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1621182702)
Concept ACK
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252685430)
>I don’t think this CPU DoS risk, if correct, is that much an increase in DoS surface, though a MAX_ANNEX_BUDGET can be a good conservative “belt-and-suspender” DoS limits, like we have a lot in Bitcoin Core.
If it isn't that much of an increase, is it worth adding extra code for it? More code, more things to go wrong. In the end, they are also paying for this tx in sats and the computation cost only increases linearly with the number of inputs.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252685430)
>I don’t think this CPU DoS risk, if correct, is that much an increase in DoS surface, though a MAX_ANNEX_BUDGET can be a good conservative “belt-and-suspender” DoS limits, like we have a lot in Bitcoin Core.
If it isn't that much of an increase, is it worth adding extra code for it? More code, more things to go wrong. In the end, they are also paying for this tx in sats and the computation cost only increases linearly with the number of inputs.
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252689789)
Yes, that's clear now. Thanks.
Same as with `MAX_ANNEX_BUDGET`, my feeling is that this safeguard isn't necessary because the extra cost is only linear and paid for.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252689789)
Yes, that's clear now. Thanks.
Same as with `MAX_ANNEX_BUDGET`, my feeling is that this safeguard isn't necessary because the extra cost is only linear and paid for.
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252693892)
>If you’re taking the use-case of unstructured data discussed on the mailing list
Which one do you refer to exactly, the timelocked vaults using presigned txes? For that there is only a single party, so the inflation attack doesn't apply?
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252693892)
>If you’re taking the use-case of unstructured data discussed on the mailing list
Which one do you refer to exactly, the timelocked vaults using presigned txes? For that there is only a single party, so the inflation attack doesn't apply?
💬 virtu commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621225530)
Even with the extra information, I'm somewhat at a loss because I cannot reproduce the problem locally. From what I understand:
- the tracepoint got called, the handler was executed
- the tx hash is correct (so `bpf_usdt_readarg_p(1, ctx, &rejected.hash, HASH_LENGTH)` in `trace_rejected` works as expected)
- reject reason is usually correct; but sometime's it's an empty string, suggesting something went wrong with `bpf_usdt_readarg_p(2, ctx, &rejected.reason, MAX_REJECT_REASON_LENGTH)` in `tr
...
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621225530)
Even with the extra information, I'm somewhat at a loss because I cannot reproduce the problem locally. From what I understand:
- the tracepoint got called, the handler was executed
- the tx hash is correct (so `bpf_usdt_readarg_p(1, ctx, &rejected.hash, HASH_LENGTH)` in `trace_rejected` works as expected)
- reject reason is usually correct; but sometime's it's an empty string, suggesting something went wrong with `bpf_usdt_readarg_p(2, ctx, &rejected.reason, MAX_REJECT_REASON_LENGTH)` in `tr
...
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1621228405)
>I think at that stage the proposal would benefit from an “applications" BIP draft that would give one or two use-cases in detail, and therefore serves as technical tie-breaker for the conceptual concerns raised above.
I agree that use cases can help guide the decision making in this PR. That's why I created the demo/explanation mentioned in the PR description: https://github.com/joostjager/annex-covenants.
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1621228405)
>I think at that stage the proposal would benefit from an “applications" BIP draft that would give one or two use-cases in detail, and therefore serves as technical tie-breaker for the conceptual concerns raised above.
I agree that use cases can help guide the decision making in this PR. That's why I created the demo/explanation mentioned in the PR description: https://github.com/joostjager/annex-covenants.
💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621281848)
Looking at the code, I fail to see how this could even pass once, because you can't assume a pointer created by `std::string{}.c_str()` can be used after the statement at all, no?
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621281848)
Looking at the code, I fail to see how this could even pass once, because you can't assume a pointer created by `std::string{}.c_str()` can be used after the statement at all, no?
📝 fanquake locked a pull request: "typos on src files"
(https://github.com/bitcoin/bitcoin/pull/28022)
Fixes multiple typos:
efficently --> efficently
succesfully --> successfully
coeffients --> coefficients
determinstic --> deterministic
initalized --> initialized
occurences --> occurrences
implemenation --> implementation
(https://github.com/bitcoin/bitcoin/pull/28022)
Fixes multiple typos:
efficently --> efficently
succesfully --> successfully
coeffients --> coefficients
determinstic --> deterministic
initalized --> initialized
occurences --> occurrences
implemenation --> implementation
💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621292840)
Or do the `TRACEx` macros create a copy of the data in-line? If yes, it could make sense to document that in doc/tracing.md#c_str
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621292840)
Or do the `TRACEx` macros create a copy of the data in-line? If yes, it could make sense to document that in doc/tracing.md#c_str
⚠️ fanquake opened an issue: "wallet_importdescriptors.py: can't decode bytes in position 228861-228863: unexpected end of data"
(https://github.com/bitcoin/bitcoin/issues/28030)
Failure in the TSAN job: https://cirrus-ci.com/task/5229709189971968?logs=ci#L1848:
```bash
node0 2023-07-04T11:34:16.737172Z [httpworker.0] [wallet/wallet.h:895] [WalletLogPrintf] [encrypted_wallet] Scanning current mempool transactions.
node0 2023-07-04T11:34:16.737361Z [httpworker.0] [wallet/wallet.h:895] [WalletLogPrintf] [encrypted_wallet] Rescan completed in 53755ms
test 2023-07-04T11:34:16.743000Z TestFramework (ERROR): Unexpected exception caught during testing
...
(https://github.com/bitcoin/bitcoin/issues/28030)
Failure in the TSAN job: https://cirrus-ci.com/task/5229709189971968?logs=ci#L1848:
```bash
node0 2023-07-04T11:34:16.737172Z [httpworker.0] [wallet/wallet.h:895] [WalletLogPrintf] [encrypted_wallet] Scanning current mempool transactions.
node0 2023-07-04T11:34:16.737361Z [httpworker.0] [wallet/wallet.h:895] [WalletLogPrintf] [encrypted_wallet] Rescan completed in 53755ms
test 2023-07-04T11:34:16.743000Z TestFramework (ERROR): Unexpected exception caught during testing
...
💬 glozow commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1252831228)
No, any parents from previous blocks would not exist in the mempool anymore. They would have been removed when that block was processed.
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1252831228)
No, any parents from previous blocks would not exist in the mempool anymore. They would have been removed when that block was processed.
💬 glozow commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1621396296)
> I have historical snapshots and would be happy to help run the test. Although, since running the test is not trivial, I wouldn't make a blocker for the PR.
Maybe something a bit simpler related to historical data: are we able to see what percentage of block transactions actually have ancestors/descendants? My understanding is that the percentage is quite low. So an alternative approach could be to just ignore any transactions with in-block descendants (just like we ignore any with in-block
...
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1621396296)
> I have historical snapshots and would be happy to help run the test. Although, since running the test is not trivial, I wouldn't make a blocker for the PR.
Maybe something a bit simpler related to historical data: are we able to see what percentage of block transactions actually have ancestors/descendants? My understanding is that the percentage is quite low. So an alternative approach could be to just ignore any transactions with in-block descendants (just like we ignore any with in-block
...
💬 willcl-ark commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621425662)
AFAIU the macros themselves don't copy any data, rather just define a static probe point. Any arguments passed to them just get incorporated into the assembly code. The tracing program (BPF) if loaded into the kernel can run code at the trace points. Here it can choose to, and AFAIU does in our case with BPF, copy the data into a different kernel space map when it hits a tracepoint, which can then be read by the USDT probe in our python test.
So the data does get copied but not by the macro,
...
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621425662)
AFAIU the macros themselves don't copy any data, rather just define a static probe point. Any arguments passed to them just get incorporated into the assembly code. The tracing program (BPF) if loaded into the kernel can run code at the trace points. Here it can choose to, and AFAIU does in our case with BPF, copy the data into a different kernel space map when it hits a tracepoint, which can then be read by the USDT probe in our python test.
So the data does get copied but not by the macro,
...
💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621437675)
Is the tracing program executed before the "full-expression" that encompasses the `std::string{}.c_str()` call has been fully evaluated?
Anything that executes after the expression has been evaluated, will operate on destroyed memory. See https://en.cppreference.com/w/cpp/language/lifetime
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621437675)
Is the tracing program executed before the "full-expression" that encompasses the `std::string{}.c_str()` call has been fully evaluated?
Anything that executes after the expression has been evaluated, will operate on destroyed memory. See https://en.cppreference.com/w/cpp/language/lifetime
💬 dergoegge commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621453405)
If we want to fuzz RPCs individually through the test runner and oss-fuzz then wouldn't it make more sense to actually have a separate target per RPC (similar to what we used to have with process_message)? Otherwise we end up with this extra test runner edge case and another oss-fuzz hack.
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621453405)
If we want to fuzz RPCs individually through the test runner and oss-fuzz then wouldn't it make more sense to actually have a separate target per RPC (similar to what we used to have with process_message)? Otherwise we end up with this extra test runner edge case and another oss-fuzz hack.
💬 craigraw commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1621466705)
Successfully tested against Sparrow Wallet's internal RPC client, Cormorant. By way of context I need this PR to make batch JSON-RPC requests as the Java client I am using requires JSON-RPC 2.0 formatted responses when a batched call is made.
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1621466705)
Successfully tested against Sparrow Wallet's internal RPC client, Cormorant. By way of context I need this PR to make batch JSON-RPC requests as the Java client I am using requires JSON-RPC 2.0 formatted responses when a batched call is made.
💬 MarcoFalke commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621466951)
> Otherwise we end up with this extra test runner edge case
Extra code needs to be written to support individual targets in the test runner. As you say this can either be achieved by compiling separate targets, or by adding code to the test runner (as is done in this patch).
Unless there are any benefits of your approach, it seems backward, because it will bring back all the issues that https://github.com/bitcoin/bitcoin/pull/27766 fixed.
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621466951)
> Otherwise we end up with this extra test runner edge case
Extra code needs to be written to support individual targets in the test runner. As you say this can either be achieved by compiling separate targets, or by adding code to the test runner (as is done in this patch).
Unless there are any benefits of your approach, it seems backward, because it will bring back all the issues that https://github.com/bitcoin/bitcoin/pull/27766 fixed.
💬 dergoegge commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621494433)
> Unless there are any benefits of your approach, it seems backward, because it will bring back all the issues that https://github.com/bitcoin/bitcoin/pull/27766 fixed.
I think the difference is that process_message has not shown to be too complex for the fuzzers. Any bug found via process_message_* was also found via process_message, so having the individual targets didn't make sense considering the overhead. For the rpc target that isn't true as we have seen with https://github.com/bitcoin/
...
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621494433)
> Unless there are any benefits of your approach, it seems backward, because it will bring back all the issues that https://github.com/bitcoin/bitcoin/pull/27766 fixed.
I think the difference is that process_message has not shown to be too complex for the fuzzers. Any bug found via process_message_* was also found via process_message, so having the individual targets didn't make sense considering the overhead. For the rpc target that isn't true as we have seen with https://github.com/bitcoin/
...
💬 MarcoFalke commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621504341)
> so to me it seems like the overhead of having the individual targets makes sense in this case.
Have you looked at the RPC generating helper? IIUC you may generate a hex-CBlock for a psbt RPC, which may lead to coverage feedback, but not actually useful coverage. I'd expect the overhead and downsides to be more pronounced for the `rpc` target than the `process_message` target. I have some ideas on how to improve the `rpc` target, but this can be done later.
Happy to close this pull, and
...
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621504341)
> so to me it seems like the overhead of having the individual targets makes sense in this case.
Have you looked at the RPC generating helper? IIUC you may generate a hex-CBlock for a psbt RPC, which may lead to coverage feedback, but not actually useful coverage. I'd expect the overhead and downsides to be more pronounced for the `rpc` target than the `process_message` target. I have some ideas on how to improve the `rpc` target, but this can be done later.
Happy to close this pull, and
...
💬 dergoegge commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621520071)
> Happy to close this pull, and apply it locally, but I think something like this is useful.
Yea, leave it open. Will test today.
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621520071)
> Happy to close this pull, and apply it locally, but I think something like this is useful.
Yea, leave it open. Will test today.