💬 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
...

💬 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.

💬 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?

📝 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

💬 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

⚠️ 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

...

💬 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.

💬 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
...

💬 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,
...

💬 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

💬 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.

💬 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.

💬 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.

💬 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/
...

💬 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
...

💬 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.

💬 willcl-ark commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621543263)
I'm probably outside of my wheelhouse now, but as I understand the tracepoint defines a place in the binary where a tracing program gets to insert and run (its own) code, if it's loaded in the kernel. The arguments to the macro are passed directly to the (assembly) code provided by the tracing program _in that expression_ and at that execution point BPF will copy values into it's own map in the kernel if a probe is set up. Other tracing programs or runtimes can choose not to copy data if they do
...

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1252954065)
Are you still working on this, or would you prefer someone to take this over?

💬 ismaelsadeeq commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1252956237)
Thank you
If the ancestors are confirmed in the previous block, and the child (with fee rate higher than ancestors score) is in the block we are processing, is that also considered CPFP? if yes then we are not removing those ancestors here right?

Also why is `/* inBlock = */true` ?

💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621555962)
> neither tx nor result go out of scope before it's called

I don't think the lifetime of `result` is relevant here. `GetRejectReason()` gives birth to a fresh temporary copy of the string.

> All that said, it's obviously still going wrong somehow...

Yeah, I am just guessing that this is a lifetime issue. It would be good to rule this out.