💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1587182546)
> > [it is trivial for every connection to probe data on the node's mempool] already the case for any txs that have been in the mempool for greater than two minutes, and any transactions that have been announced to a peer. Generally that's plenty for fingerprinting -- if you announce pairs of conflicting txs randomly to different nodes, then if you do that 20 times at the same low-ish feerate (so that neither will rbf the other), then wait two minutes, you'll likely already get a nearly unique
...
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1587182546)
> > [it is trivial for every connection to probe data on the node's mempool] already the case for any txs that have been in the mempool for greater than two minutes, and any transactions that have been announced to a peer. Generally that's plenty for fingerprinting -- if you announce pairs of conflicting txs randomly to different nodes, then if you do that 20 times at the same low-ish feerate (so that neither will rbf the other), then wait two minutes, you'll likely already get a nearly unique
...
💬 MarcoFalke commented on issue "CI failure: `ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const`":
(https://github.com/bitcoin/bitcoin/issues/23366#issuecomment-1587218489)
Maybe we can set https://en.cppreference.com/w/cpp/io/ios_base/sync_with_stdio as a work-around to get thread-safety?
(https://github.com/bitcoin/bitcoin/issues/23366#issuecomment-1587218489)
Maybe we can set https://en.cppreference.com/w/cpp/io/ios_base/sync_with_stdio as a work-around to get thread-safety?
💬 fanquake commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1587230845)
I think it's ok to merge this now.
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1587230845)
I think it's ok to merge this now.
🚀 fanquake merged a pull request: "validation: Move warningcache to ChainstateManager and rename to m_warningcache"
(https://github.com/bitcoin/bitcoin/pull/27357)
(https://github.com/bitcoin/bitcoin/pull/27357)
💬 stickies-v commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226635021)
Why do we need this repeated test? At the very least it seems orthogonal to the PR?
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226635021)
Why do we need this repeated test? At the very least it seems orthogonal to the PR?
💬 stickies-v commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226624400)
We don't do any named argument transformation when calling `getdeploymentinfo().HandleRequest(jsonRequest)` directly, so the `jsonRequest` is passed as is. If you look at the implementation of `getdeploymentinfo()`, you'll see it fetches the argument by position: `request.params[0]`.
Even though this implementation works, it's pretty fragile: it would also work if you'd used `jsonRequest.params.pushKV("not-a-blockhash", hash_str);`, for example.
I think sticking to a `VARR` is best:
```
...
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226624400)
We don't do any named argument transformation when calling `getdeploymentinfo().HandleRequest(jsonRequest)` directly, so the `jsonRequest` is passed as is. If you look at the implementation of `getdeploymentinfo()`, you'll see it fetches the argument by position: `request.params[0]`.
Even though this implementation works, it's pretty fragile: it would also work if you'd used `jsonRequest.params.pushKV("not-a-blockhash", hash_str);`, for example.
I think sticking to a `VARR` is best:
```
...
⚠️ fanquake opened an issue: "tsan: failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/issues/27860)
master @ 6f5f37eefd425faabd9a97a3c3028395c34f08c4.
https://cirrus-ci.com/task/5086176751648768?logs=ci#L3409:
```bash
node0 2023-06-12T12:48:24.904003Z [msghand] [txmempool.cpp:1011] [RemoveUnbroadcastTx] [mempool] Removed 0668eb571427ae4ed5ee1f9d9d3d0c6de4261bba44335d033a75be0183d87a46 from set of unbroadcast txns
test 2023-06-12T12:48:24.952000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
...
(https://github.com/bitcoin/bitcoin/issues/27860)
master @ 6f5f37eefd425faabd9a97a3c3028395c34f08c4.
https://cirrus-ci.com/task/5086176751648768?logs=ci#L3409:
```bash
node0 2023-06-12T12:48:24.904003Z [msghand] [txmempool.cpp:1011] [RemoveUnbroadcastTx] [mempool] Removed 0668eb571427ae4ed5ee1f9d9d3d0c6de4261bba44335d033a75be0183d87a46 from set of unbroadcast txns
test 2023-06-12T12:48:24.952000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
...
💬 dergoegge commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587311912)
> having it implemented into bitcoind is not a big change and will allow more flexibility in when templates are pushed respect to just poll getblocktemplate
What are you thinking of in terms of flexibility that the current interfaces do not offer? I can't think of anything that would not work by using the current interfaces or exposing extra functionality through them. For example, looking at Matt's comment https://github.com/bitcoin/bitcoin/pull/23049#issuecomment-926009122:
> do things l
...
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587311912)
> having it implemented into bitcoind is not a big change and will allow more flexibility in when templates are pushed respect to just poll getblocktemplate
What are you thinking of in terms of flexibility that the current interfaces do not offer? I can't think of anything that would not work by using the current interfaces or exposing extra functionality through them. For example, looking at Matt's comment https://github.com/bitcoin/bitcoin/pull/23049#issuecomment-926009122:
> do things l
...
📝 TheCharlatan opened a pull request: "kernel: Add interrupt class"
(https://github.com/bitcoin/bitcoin/pull/27861)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent
...
(https://github.com/bitcoin/bitcoin/pull/27861)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent
...
📝 TheCharlatan converted_to_draft a pull request: "kernel: Remove shutdown globals from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27711)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
This removes the shutdown files from the kernel library. As a standalone library the libbitcoinkernel should not have to rely on code that accesses shutdown state through a global. Instead, replace it with an interrupt class that is owned by the k
...
(https://github.com/bitcoin/bitcoin/pull/27711)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
This removes the shutdown files from the kernel library. As a standalone library the libbitcoinkernel should not have to rely on code that accesses shutdown state through a global. Instead, replace it with an interrupt class that is owned by the k
...
💬 stickies-v commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1587332103)
Rebased to address merge conflict from https://github.com/bitcoin/bitcoin/pull/27844.
> It would also be nice to have an incantation (e.g. in the help) to kill dangling images after CI failure / interruption (if you don't plan on running it again).
Thanks, added [doc: ci: add instructions for pruning dangling images manually](https://github.com/bitcoin/bitcoin/pull/27793/commits/f9b5fe40cdaab04a1bf72699f1eb4be44ab24bbe)
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1587332103)
Rebased to address merge conflict from https://github.com/bitcoin/bitcoin/pull/27844.
> It would also be nice to have an incantation (e.g. in the help) to kill dangling images after CI failure / interruption (if you don't plan on running it again).
Thanks, added [doc: ci: add instructions for pruning dangling images manually](https://github.com/bitcoin/bitcoin/pull/27793/commits/f9b5fe40cdaab04a1bf72699f1eb4be44ab24bbe)
💬 brunoerg commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226659147)
I did it to ensure it is fetching data for old blocks correctly.
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226659147)
I did it to ensure it is fetching data for old blocks correctly.
👍 ryanofsky approved a pull request: "Raise on invalid -debug and -loglevel config options"
(https://github.com/bitcoin/bitcoin/pull/27632#pullrequestreview-1474878314)
Code review ACK 1a27bec1e9e40166d6ff7f0492115107289e7609. I left a review comment about this adding some argument names to translated strings, which is bad but already happens other places, so it might not be very important to fix.
I do think it would be good if this PR added release notes, because the changes are not backwards compatible and will probably break some existing configurations or misconfigurations.
It also looks like this has had enough code review to be ready to merge. @jona
...
(https://github.com/bitcoin/bitcoin/pull/27632#pullrequestreview-1474878314)
Code review ACK 1a27bec1e9e40166d6ff7f0492115107289e7609. I left a review comment about this adding some argument names to translated strings, which is bad but already happens other places, so it might not be very important to fix.
I do think it would be good if this PR added release notes, because the changes are not backwards compatible and will probably break some existing configurations or misconfigurations.
It also looks like this has had enough code review to be ready to merge. @jona
...
💬 ryanofsky commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1226635145)
In commit "init: raise on invalid debug/debugexclude config options" (398aa75f94114684dfee5ea5a2bc9ebe0860bb77)
This commit is adding command line argument names to a translated string, which is something we want to avoid to avoid translation errors. See #20404. Would be better to leave the previous strprintf in place.
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1226635145)
In commit "init: raise on invalid debug/debugexclude config options" (398aa75f94114684dfee5ea5a2bc9ebe0860bb77)
This commit is adding command line argument names to a translated string, which is something we want to avoid to avoid translation errors. See #20404. Would be better to leave the previous strprintf in place.
💬 brunoerg commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226664781)
Agreed, gonna address it!
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226664781)
Agreed, gonna address it!
💬 brunoerg commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#issuecomment-1587345939)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226624400
(https://github.com/bitcoin/bitcoin/pull/27853#issuecomment-1587345939)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226624400
💬 Fi3 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587347653)
Can not answer about Matt comment. (@TheBlueMatt maybe can clarify?)
What I intended is for example create a new template as soon as txs in mempool allow creations of new template with a fee amount that is bigger than x than the one of last created template. Or create a future template based on the one that miner is mining (next-next-block).
> This is not something we currently support and would (afaik) require significant changes to our mining/mempool logic (entirely separate of any str
...
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587347653)
Can not answer about Matt comment. (@TheBlueMatt maybe can clarify?)
What I intended is for example create a new template as soon as txs in mempool allow creations of new template with a fee amount that is bigger than x than the one of last created template. Or create a future template based on the one that miner is mining (next-next-block).
> This is not something we currently support and would (afaik) require significant changes to our mining/mempool logic (entirely separate of any str
...
💬 brunoerg commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1587351546)
Rebased
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1587351546)
Rebased
💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1587357662)
reACK https://github.com/bitcoin/bitcoin/pull/26969/commits/89478af481b2e6d4652b2fe50ff5fe5b92cbd69e
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1587357662)
reACK https://github.com/bitcoin/bitcoin/pull/26969/commits/89478af481b2e6d4652b2fe50ff5fe5b92cbd69e
👍 ryanofsky approved a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1474958262)
Code review ACK 1a27bec1e9e40166d6ff7f0492115107289e7609.
This should be a really nice change because it actually makes `bitcoind` return an error code when there is an error. But it also clears out `AbortNode` clutter and makes the `main()` function comprehensible.
I think this would be ready for merge if another reviewer added an ACK.
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1474958262)
Code review ACK 1a27bec1e9e40166d6ff7f0492115107289e7609.
This should be a really nice change because it actually makes `bitcoind` return an error code when there is an error. But it also clears out `AbortNode` clutter and makes the `main()` function comprehensible.
I think this would be ready for merge if another reviewer added an ACK.