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

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

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

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

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

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

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

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

💬 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

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

💬 brunoerg commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(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

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

💬 stickies-v commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226690678)
Yeah I get that, but I don't see why we wouldn't just run the hash-specified test immediately on the older hash to avoid doing this twice? Also just querying the previous hash seems more efficient than running generate?

Upon further thought, I think it does indeed make sense to include it in this PR.

```diff
diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py
index 559249101..1ba8f60d9 100755
--- a/test/functional/interface_rest.py
+++ b/test/functional/i
...

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1226708783)
IIRC circles back to the diamond problem: https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1542695316

Both the parents should contribute to the grandparent being included, but in ancestor packages we can't take that into account(without a common descendant). Instead the whole ancestor package is included, even though the child is likely to be immediately evicted.

💬 ismaelsadeeq commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1587414647)
Post Merge code review ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4

This PR removes `mapRelay` and introduces `m_recent_block_txs`.
`m_recent_block_txs` retains the transactions of the most recent mined block.
When a new block is mined, all the block transactions in our mempool are dropped, because of `BLOCK` `MemPoolRemovalReason`.

- Storing the most recently mined block transactions in `m_recent_block_txs` allows us to relay the dropped transactions to peers upon request, which we mig
...

💬 sipa commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587485219)
I'm Concept ACK on eventually replacing the current `getblocktemplate` with another interface, for the following reasons:
* The poll-based nature introduces unnecessary latency over a hypothetical push-based one.
* Being RPC based, `getblocktemplate` is bandwidth and CPU inefficient due to JSON and hex encoding (for large amounts of data being transferred on every call).
* `getblocktemplate` provides too much information for clients that do not care about performing their own transaction sele
...

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226788428)
Why did you add one more node?

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1587500451)
> So we would still need to track the elapsed time to not run the check more often than necessary. So I think just using the 24h scheduler is cleaner and just as good.

I agree.

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226805905)
Since `test_asmapp_health_check` is called after `test_empty_asmap`, node0 is not running, so we don't need to stop it here again.