👍 stickies-v approved a pull request: "test: Do not pass tests on unhandled exceptions"
(https://github.com/bitcoin/bitcoin/pull/33001#pullrequestreview-3030467318)
ACK fa30b34026f76a5b8af997152fced2d281782e0d

🤔 ismaelsadeeq reviewed a pull request: "ipc: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-3029237466)
Weak ACK 2910315d1d8a3879f4e0bd50bf90870b642bfede

I’m not sure whether 600a52a0ba08dca06de891e6716ab2730580ddcd is necessary.

Personally, I preferred the previous iteration keeping this PR as a minimal boilerplate to demonstrate IPC usage. This increases the scope quite a bit, and if we're going to demonstrate mining, I think it should be done accurately.

I’ve added some comments below pointing out that we don’t currently handle chain tip changes. Could also be better to use `waitNext`
...

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213158587)
nit:
```suggestion
# Set paths to bitcoin core binaries allowing overrides with environment
```

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213893768)
Return when we are not on test chain?

```suggestion
if (!Params().IsTestChain()) return EXIT_SUCCESS;

```

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213909058)
only send solution when you have a valid block?
```suggestion
if (CheckProofOfWork(block.GetHash(), block.nBits, consensus_params)) block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, block_template->getCoinbaseTx());

```

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213949664)
The logs are really verbose and clunky I opened a seperate issue https://github.com/bitcoin-core/libmultiprocess/issues/190

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213947741)
This should also break when the chain tip changes or generate a new template. Right now, it's possible to pass a large `max_tries` and get stuck.

```suggestion
--tries_remaining;
if (block.hashPrevBlock != mining->getTip()->hash) {
block_template{mining->createNewBlock({})};
block{new_block_template->getBlock()};
}
```

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213913656)
We are on regtest shouldn't we always mine a block?
```suggestion
expect = r"Connected to bitcoin-node\nTip hash is 0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206\.\nMined a block.*?\n"

```

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#issuecomment-3084977883)
Thanks for the review @glozow! I've addressed your comments and also refactored a few of the functional tests.

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213986455)


cc @TheCharlatan since you wrote https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2984042087

Just resurfacing a previous comment along with a recent one.

I think this should remain a simple IPC boilerplate. You mentioned “it's quite a bit” perhaps you should indicate what you feel is unnecessary and suggest ways we could slim it down.

@Sjors:

> Binary focused on testing / demoing the mining interface.

Don’t we already have unit tests for the interface? If the goal
...

💬 Sjors commented on issue "Guix build fails on M4 macOS host with Ubuntu in UTM":
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3085012171)
Yes that was needed... Afterwards:

```
$ guix --version
guix (GNU Guix) 945c6ff9f222efed460ce8ab31fb6911f89436c9
...
guix install python-lief@0.16.6 --no-substitutes
...

building /gnu/store/1krys8fl33k7bzr9md8fgd38k41av1fk-babel-2.16.0.tar.gz.drv...
guix install: error: while setting up the build environment: cannot set armhf-linux personality: Invalid argument
```

💬 PeterWrighten commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3085026791)
> You'll have to rebase. Did you see the immediately previous comment? Also, ref https://github.com/bitcoin/bitcoin/pull/32818

I have rebased and read comment. I consider my implementation is better...

📝 marcofleon opened a pull request: "refactor: GenTxid type safety followups"
(https://github.com/bitcoin/bitcoin/pull/33005)
This is a followup to https://github.com/bitcoin/bitcoin/pull/32631.

Addresses:
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2200291621
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2199951996
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201874252
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201918072
https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-3063383775

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2214063550)
thanks, added to #32941

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2214063681)
thanks, added to #32941

💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3085123179)
@JeremyRubin in your [comment](https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-3063383775) you mentioned that there are multiple data structures that should be switched to homogeneous. I was only able to spot `m_tx_inventory_to_send` as one that should be changed. Are there others that I'm missing?

If it is only the one, then I'm not sure about making such a generic container. Might be better to just keep it specific to Txid/Wtxid or have a class for `m_tx_inventory_to_send` only.
...

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2214083519)
Thanks again it's simpler like this, done in https://github.com/bitcoin/bitcoin/pull/24539/commits/42d3aa8787d9411ad2a9c9ae9c828aa1285853ec

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2214083883)
done in done in https://github.com/bitcoin/bitcoin/pull/24539/commits/42d3aa8787d9411ad2a9c9ae9c828aa1285853ec

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2214085643)
No we just need to skip over it, done in https://github.com/bitcoin/bitcoin/pull/24539/commits/42d3aa8787d9411ad2a9c9ae9c828aa1285853ec

💬 brunoerg commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#discussion_r2214109577)
Yes, it's possible. I just addressed it.