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

💬 brunoerg commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3085207500)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/33003#discussion_r2213532921

🤔 mzumsande reviewed a pull request: "mempool: Avoid needless vtx iteration during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3030644452)
Code Review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50

I think it makes sense not to do these iterations over block txns if these can never result in any action - even if it doesn't increase performance measurably.

💬 mzumsande commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214057025)
nit: If `mapTx` is empty, `mapNextTx` must necessarily be empty as well, so strictly speaking it doesn't need to be checked here.