💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2213916090)
Yes, this was an outdated comment which I've now removed

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2213916340)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2213917014)
Yes, I've updated both of the places where I was doing this.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2213923610)
Yes, because when Bob confirms, it is removed from `mempool_conflicts` and Alice's transaction is considered "Inactive". I've added a part to this test case where Alice evicts Bob's transaction.

💬 sfsegreto commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3084894912)
Can you be even more specific please? When you say cross-compilation do you mean cross-compile from Linux to Windows or do you mean cross-compile from Winx64 to WinArm64? And also what toolchain specifically does not exist?

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2213927081)
It would be tricky to consolidate the two because one of them is testing preventing the wallet from creating a transaction and the other is testing marking conflicts correctly.

💬 instagibbs commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3084911947)
Having issues getting this to work on my local Verizon router. I don't appear to be reachable at the port specified and I don't see any pmp fallbacks happening in logs?

Every 5 minutes I see this set of logs (with port 8339 fwiw):

2025-07-17T17:35:03.446228Z [net] portmap: gateway [IPv4]: <redacted>
2025-07-17T17:35:03.446327Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 8339 from gateway <redacted>
2025-07-17T17:35:03.446407Z [net] pcp: Internal address after connect: <redacted>
2
...

💬 stickies-v commented on pull request "test: Do not pass tests on unhandled exceptions":
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213942752)
> You can try this for any command that captures the output:

Ah cool, thanks for the example. Agreed that improving is out of scope for this PR then.

> I don't think it makes sense to show the traceback when the user pressed CTRL+C?

No strong preference either way, I kinda like seeing where it got interrupted (and I'm already used to it because the `KeyboardInterrupt` exception handling doesn't work for test_runner.py which is my usual interface), but makes sense to not change this beha
...

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