⚠️ 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.
💬 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
...
(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?
(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` ?
(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.
(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.
👍 TheCharlatan approved a pull request: "test: bugfix, synchronize indexes synchronously"
(https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1514306173)
ACK f01cb3a8dcc12954d5b6e075ebeb94c00fd37779
(https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1514306173)
ACK f01cb3a8dcc12954d5b6e075ebeb94c00fd37779
💬 MarcoFalke commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1621578293)
(An alternative, functionally equivalent to this, might be to just make the timeout infinite, see https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007, but I think this is fine as well.)
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1621578293)
(An alternative, functionally equivalent to this, might be to just make the timeout infinite, see https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007, but I think this is fine as well.)
💬 MarcoFalke commented on pull request "test: Fix intermittent issue in mining_getblocktemplate_longpoll.py":
(https://github.com/bitcoin/bitcoin/pull/27941#issuecomment-1621604286)
In any case, if the Bitcoin Core behavior is changed, it should be accompanied with deterministic tests. I don't think non-deterministic tests are useful, especially if they lead to a failure every time they hit the "other side" of the race.
Going to reopen for now to fix the intermittent issue, and make the tests deterministic. Happy to review a future or conflicting pull that instead changes the non-test code, or if there is documentation I missed.
(https://github.com/bitcoin/bitcoin/pull/27941#issuecomment-1621604286)
In any case, if the Bitcoin Core behavior is changed, it should be accompanied with deterministic tests. I don't think non-deterministic tests are useful, especially if they lead to a failure every time they hit the "other side" of the race.
Going to reopen for now to fix the intermittent issue, and make the tests deterministic. Happy to review a future or conflicting pull that instead changes the non-test code, or if there is documentation I missed.
📝 MarcoFalke reopened a pull request: "test: Fix intermittent issue in mining_getblocktemplate_longpoll.py"
(https://github.com/bitcoin/bitcoin/pull/27941)
Fixes https://github.com/bitcoin/bitcoin/issues/26962
Wait for the thread to have started and the RPC to have reached the node before continuing. Otherwise the test may run into a race.
For example:
```
test 2023-06-23T13:10:29.245000Z TestFramework (INFO): Test that introducing a new transaction into the mempool will terminate the longpoll
node0 2023-06-23T13:10:29.245712Z [http] [httpserver.cpp:254] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:43568
n
...
(https://github.com/bitcoin/bitcoin/pull/27941)
Fixes https://github.com/bitcoin/bitcoin/issues/26962
Wait for the thread to have started and the RPC to have reached the node before continuing. Otherwise the test may run into a race.
For example:
```
test 2023-06-23T13:10:29.245000Z TestFramework (INFO): Test that introducing a new transaction into the mempool will terminate the longpoll
node0 2023-06-23T13:10:29.245712Z [http] [httpserver.cpp:254] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:43568
n
...
💬 Ayush170-Future commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1621635810)
A gentle ping to @luke-jr, what about this issue then? [23589](https://github.com/bitcoin/bitcoin/issues/23589)
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1621635810)
A gentle ping to @luke-jr, what about this issue then? [23589](https://github.com/bitcoin/bitcoin/issues/23589)