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

💬 brunoerg commented on pull request "doc: clarify GetAddresses documentation":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3085283292)
I agree with @stickies-v. It's weird to have two functions with the same name but with a relevant behavior difference, renaming one of the functions seems appropriate. In any case, Concept ACK about improving the documentation.

💬 kurapika007 commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3085325955)
> > It's not obvious that the price going up 10x means we get to lower the default by 10x, because it's a more important system now and the stakes are simply higher.
>
> Sure, though at the same time the situation being discussed here isn't price up 10x means default down 10x. Compared to when this was last adjusted the price is up 500x and this proposes down by 10x. :P The magnitudes matter-- particularly in that it answers your drawdown comment: Even if bitcoin prices drop by 90% they'll st
...