💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967421)
I'm now setting `uint256 tip{miner.getTipHash()};` at the top. For long polls it's updated after the wait. See d1be135e568605539fa3932da6e7c5032816825b.

💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967487)
Renamed to `Mining`

💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967663)
I replaced `getTip()` with `getTipHash()`. `Sv2TemplateProvider` doesn't need it, so for now I use it in a few places in `rpc/mining.cpp`, but then fall back to `chainman.m_blockman.LookupBlockIndex(miner.getTipHash())` where we need more info.

A future refactor could move more of `getblocktemplate` implementation details out of the RPC.

💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967783)
Thanks, replaced it with `get()`.

💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2154504861)
@ryanofsky thanks for the review! I rebased and changed a few things to address comments, see inline.

Also added `IsInitialBlockDownload`, which was the last thing `Sv2TemplateProvider` still needed `ChainstateManager& m_chainman;` for.

I'll cherry-pick this into #29432 to see if we need anything more.

📝 Sjors converted_to_draft a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200)
Introduce a `Mining` interface for the `getblocktemplate` and `generateblock` RPC to use now, and for Stratum v2 to use later.

Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652

The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.

This PR should be a pure refactor.

💬 glozow commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2154575591)
Concept ACK

> > Not sure if that is worth addressing since it's so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
>
> I think it's worth avoiding, I don't really like the notion of flaky tests maybe failing for a random reason, even if it is very improbable. I'd guess short-ID collisions would also affect some of the other existing tests in here so there's additional benefit in them not being flaky too.

Not sure exactly how flaky it is and don
...

🤔 shaavan reviewed a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-2104235508)
ACK 659663af32f02c570e334e8f375fd5f5737258d7 ⚡️

📝 Eunovo opened a pull request: "Tr partial descriptors"
(https://github.com/bitcoin/bitcoin/pull/30243)
This PR adds support for rawnode() and rawleaf() descriptors discussed in #24114 and [tr-rawnode-and-rawleaf](https://delvingbitcoin.org/t/tr-rawnode-and-rawleaf-support/901)

📝 Eunovo converted_to_draft a pull request: "Tr partial descriptors"
(https://github.com/bitcoin/bitcoin/pull/30243)
This PR adds support for rawnode() and rawleaf() under tr() descriptor discussed in #24114 and [tr-rawnode-and-rawleaf](https://delvingbitcoin.org/t/tr-rawnode-and-rawleaf-support/901)

💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2154691207)
Thank you for your suggestions @ryanofsky! I applied both of your suggested patches.

eeea0818c1a20adc5225b98b185953d386c033e0 -> 682f1f1827dff78208bc1272f82d217c42a2cd69 ([preserveIndexOnRestart_4](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_4) -> [preserveIndexOnRestart_5](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/preserveIndexOnRestart_4..preserveIndexOnRestart_5))

* Addressed
...

👍 hebasto approved a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2104389645)
re-ACK 7b8eea067f188c0b0e52ef21b01aedd37667a237, I've verified changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2103018546) with
```
git range-diff master ee253ca7dea9bed01d4c1800760477ef06310df8 7b8eea067f188c0b0e52ef21b01aedd37667a237
```

📝 m3dwards opened a pull request: "ci: parse TEST_RUNNER_EXTRA into an array"
(https://github.com/bitcoin/bitcoin/pull/30244)
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.

This change allows proper parsing of quotes and complex values such as:

```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6, feature_proxy.py"'
```

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2154727588)
reACK https://github.com/bitcoin/bitcoin/pull/30161/commits/7b8eea067f188c0b0e52ef21b01aedd37667a237

reviewed via `git range-diff master ee253ca 7b8eea0`

💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631127412)
I don't think this works, does it? installation happens when the image is built (`docker build`), not when the tests are compiled and run.

See the Cirrus CI comment:

```
# In the image build step, no external environment variables are available,
# so any settings will need to be written to the settings env file:

🤔 maflcko requested changes to a pull request: "ci: move ASAN job to GitHub Actions from Cirrus CI"
(https://github.com/bitcoin/bitcoin/pull/30193#pullrequestreview-2104414380)
lgtm, except for the feedback

💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631125417)
Please clarify in the name that this was set up for CI

💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631127593)
?

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2154745368)
> `git ls-files` is also used in `./test/lint/lint-python.py`, for reference. So using `git ls-files -- '*.md'` and passing the full array to mlc may be the best approach for us?

Thanks, I changed the approach in https://github.com/willcl-ark/mlc/tree/git-aware to use `git status --ignored *.md` and append to the ignored paths, as adding to that felt like the path of least resistance.

I will investigate if I can have a `--git` flag only check the result of `git ls-files -- '*.md'`, as I fe
...

💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631137020)
Also, it could split this up into a separate commit/pull, as this affects more than just this one task?