💬 maflcko commented on pull request "build: Drop no longer needed MSVC warning suppressions":
(https://github.com/bitcoin/bitcoin/pull/28798#issuecomment-1794394106)
lgtm ACK 33223f9d550b66885a15e2fe45147d629cc359d2

✅ maflcko closed a pull request: "RPC/Wallet: Access wallets via interfaces::Wallet"
(https://github.com/bitcoin/bitcoin/pull/26082)

💬 maflcko commented on pull request "RPC/Wallet: Access wallets via interfaces::Wallet":
(https://github.com/bitcoin/bitcoin/pull/26082#issuecomment-1794410487)
Closing for now. Please leave a comment here, to have it re-opened by someone.

🤔 Sjors reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1714571006)
Concept ACK. Lightly reviewed the refactoring (as of a20c1d2b81a702f97267d4f098f13ed80e9320cf) which all looks reasonable, but I'm not very familiar with mempool code.

💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382967058)
ee79f05f1fef57bfbea24333a46e525385d02319: `{`

💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382997719)
6b6793e6835dd2ddf367490362042bf34eb260a3: The only caller `checkChainLimits` of is the wallet's `CreateTransactionInternal`, which will say "Transaction has too long of a mempool chain" regardless of which limit is hit.

A (wallet) followup should propagate `err_string` up.

💬 maflcko commented on pull request "Fix typos and suggest doing so before branch-off":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794414689)
Are you still working on this? Otherwise, I think it can be closed, so that it can be picked up by someone else, if it is still relevant.

💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383019317)
ee79f05f1fef57bfbea24333a46e525385d02319: this fixes the dangling warning:

```cpp
const CTxMemPoolEntry& entry{Assert(m_pool.GetEntry(txid)).value()};
```

💬 Sjors commented on pull request "Fix typos and suggest doing so before branch-off":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794431063)
I was still waiting for feedback from maintainers, but I'll just drop the last commit.

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383022517)
Thank, might be a good idea to bracket initialize in other places too.

💬 willcl-ark commented on pull request "doc: Add offline signing tutorial":
(https://github.com/bitcoin/bitcoin/pull/28363#issuecomment-1794436797)
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33

Looks good to me!

💬 dergoegge commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1794443638)
Here's a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28578/fuzz.coverage/index.html (~1000h CPU time).

Is there a reason not to cover more of `DescriptorScriptPubKeyMan` in this PR? e.g. `DescriptorScriptPubKeyMan::SignTransaction` or `DescriptorScriptPubKeyMan::FillPSBT`.

🚀 fanquake merged a pull request: "build: Drop no longer needed MSVC warning suppressions"
(https://github.com/bitcoin/bitcoin/pull/28798)

💬 fanquake commented on pull request "Fix typos":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794459605)
```bash
test/functional/feature_assumeutxo.py:78: refering ==> referring
test/functional/feature_assumeutxo.py:115: refering ==> referring
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```

👍 dergoegge approved a pull request: "fuzz: Avoid utxo_total_supply timeout (take 2)"
(https://github.com/bitcoin/bitcoin/pull/28789#pullrequestreview-1714698328)
ACK fa7ba926300f44b350d04262e569cc607e4991d8

💬 maflcko commented on pull request "Fix typos":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794494686)
> I was still waiting for feedback from maintainers

DrahtBot has a summary comment, which has links to all feedback, including that of maintainers, see https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1750139214

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794513714)
Updated a20c1d2b81a702f97267d4f098f13ed80e9320cf -> 836a0a7566e166556f80303a9b3dd9560587159c ([simplifyMemPoolInteractions_7](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_7) -> [simplifyMemPoolInteractions_8](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_7..simplifyMemPoolInteractions_8))

* Addressed @Sjors's [comment](https://github.com/bitcoin/bit
...

🤔 ismaelsadeeq reviewed a pull request: "MiniMiner changes for package linearization"
(https://github.com/bitcoin/bitcoin/pull/28762#pullrequestreview-1714693345)
Strong ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9

The whole mempool transactions can also be used to create `manual_entries` and `descendant_caches` and use the new constructor to build a block template to get a fee rate histogram of the block template, with just a modification to `BuildMockTemplate` to enable getting the block template fee rate histogram.
it will be more desirable to modify this than `BlockAssembler`.

Due to the use of `GatherClusters`, which has a 500 transaction D
...

💬 ismaelsadeeq commented on pull request "MiniMiner changes for package linearization":
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1383041819)
I think just stating that this is a constructor where `MiniMinerMempoolEntry` are constructed manually is enough as this can be used with mempool transactions, but with the intent of building a block template to get a mempool fee rate histogram
```suggestion
* It is assumed that all
```

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794571887)
Updated 836a0a7566e166556f80303a9b3dd9560587159c -> 11b8735852bd7439c4a07c88d07d7dfb13a3c7b7 ([simplifyMemPoolInteractions_8](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_8) -> [simplifyMemPoolInteractions_9](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_8..simplifyMemPoolInteractions_9))

* Fixed msvc build error by using `auto` instead of `CTxMemPo
...