💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018254263)
Thread https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018038275:

It would be good if all `assert_` functions supported error messages explaining why the condition should never fail, or what could be wrong if it has failed. (Built-in `assert` supports it too).

To avoid mixing the error message up with a value to be checked, one could do:
```suggestion
def assert_not_equal(thing1, thing2, *, message=None):
if thing1 == thing2:
raise AssertionError(f"Both values ar
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2018313316)
Will take if anything else forces me to re-touch.

💬 Lagrang3 commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2760837538)
Router: Archer AX1800
OS: Debian 12
Bitcoin: v29.0rc2
Result: success

```bash
$ bitcoind -natpmp=1 -debug=net
...
2025-03-28T10:11:15Z [net] pcp: Received response of 8 bytes: [removed]
2025-03-28T10:11:15Z [net] portmap: Got unsupported PCP version response, falling back to NAT-PMP
2025-03-28T10:11:15Z [net] natpmp: Requesting port mapping port 8333 from gateway 192.168.0.1
2025-03-28T10:11:15Z [net] natpmp: Received response of 12 bytes: [removed]
2025-03-28T10:11:15Z [net] natpmp: Received r
...

💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2760846407)
@maflcko / @ryanofsky, this implementation seems to fail in the same weird way as https://github.com/bitcoin/bitcoin/actions/runs/12918565902/job/36027337187?pr=31539 - with 100% tests passing.
The last PR indicates it's the batch writing part - could it be an OOM because the sanitizer extras in `DataStream#resize(1 << 20)`?
`WriteBlockBench` indicates that even a buffer size of `1 << 16` is half the speed (Linux/GCC speed is the same) - but I'll try it, let's see if it passes CI at least.

💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2760865175)
> Neat! Please open a PR.

Hmmm, further testing this morning reveals other potential (root-causes and) fixes. Seems that I have two other issues with the original code:

1. Responses can be longer than 4096 bytes. I guess tailscale is filling them up!
2. Sometimes the **first** `RTA_GATEWAY` response found (which we use) may not be for the default route (and may not support pinholing). This happens for me on IPV6 only

An alternative fix therefore is to iterate over the full response, and filte
...

💬 maflcko commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018338475)
> assert_not_equal(main_block_hash, stale_block_hash, message="node 0 chain did not reorganize")

I'd say it is already self-explanatory to see `assert_not_equal(main_block_hash, stale_block_hash)` and the message "AssertionError: Both values are (the same)". It should be clear that the chains are the same, when they should be different.

Even more so, given that the test logs before the assert: `self.log.info("Reorg node 0 to a new chain.")`.

Unique and descriptive error messages make se
...

💬 i-am-yuvi commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2760925328)
I have tested on my local machine and here are some of my observation
https://github.com/i-am-yuvi/bitcoin-core-issues-prs-notes/blob/master/29.0rc_testing.md

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2018389378)
Pushed new version that hopefully clarifies things:
https://github.com/bitcoin/bitcoin/blob/7aa397a7d45ce0ae1b83cc6f3a65db0e41b21ee8/src/script/descriptor.cpp#L1442-L1447

💬 maflcko commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2760955554)
Does CI pass locally before and after? Measuring locally, what is the memory usage of the CI before and after?


It seems odd that using exact block sizes (this pull initially) passes, whereas using a `1<<20` approximation fails due to OOM.

💬 janb84 commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2760963857)
> I have tested on my local machine and here are some of my observation https://github.com/i-am-yuvi/bitcoin-core-issues-prs-notes/blob/master/29.0rc_testing.md

Thanks for the report ! Would you be so kind to make an extract from the PCP / NAT-PMP part your findings to a comment [here](https://github.com/bitcoin/bitcoin/issues/31663). Would be so helpful !

Thanks again

💬 Prabhat1308 commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2760973175)
> I have tested on my local machine and here are some of my observation https://github.com/i-am-yuvi/bitcoin-core-issues-prs-notes/blob/master/29.0rc_testing.md

You might have missed building with 29.0 . I Don't see a `reject-detail` field in the last response you have tested.

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018425335)
> "AssertionError: Both values are (the same)"

I don't know if you mean literally `"Both values are (the same)"` or `f"Both values are {thing1}"`. I find the latter clearly more useful; providing clues of cause of the failure, making it easier to reproduce.

---

What it comes down to with custom error messages is being able to directly communicate with whoever is troubleshooting a failure. If one doesn't even need to go to the logs to see what happened before, I consider it a small win.
...

✅ willcl-ark closed an issue: "Failed transactions on importing mempool"
(https://github.com/bitcoin/bitcoin/issues/32125)

💬 willcl-ark commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2761030198)
Going to close this as I don't think there's anything left to do here. Let me know if I've misunderstood if you want it re-opened.

💬 i-am-yuvi commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2761045881)
> > I have tested on my local machine and here are some of my observation https://github.com/i-am-yuvi/bitcoin-core-issues-prs-notes/blob/master/29.0rc_testing.md
>
> You might have missed building with 29.0 . I Don't see a `reject-detail` field in the last response you have tested.

Thanks for this @Prabhat1308, for some reason, while testing the updated rp,c I checked out v28.1rc. I have updated the report!

💬 maflcko commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018497295)
> Don't know if you mean literally `"Both values are (the same)"` or `f"Both values are {thing1}"`. Find the latter clearly more useful; providing clues of cause of the failure, making it easier to reproduce.

Agree. Sorry for being unclear, I meant `f"Both values are {thing1}"` when I said "(the same)".

🤔 polespinasa reviewed a pull request: "miner: timelock the coinbase to the mined block's height"
(https://github.com/bitcoin/bitcoin/pull/32155#pullrequestreview-2724722238)
Concept ack
Did a simple test with regtest and the blocks are correctly mined with a coinbase tx setting `nLockTime=heigh-1` and `nSequence=4294967294`.

I agree adding this before the soft-fork is good so we give time to miners.
I have one question although, if I'm not wrong the node miner code is no longer used in mainnet so this PR only affects to test networks?

💬 polespinasa commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2018171982)
Is this Assert necessary? Is it even possible? Genesis block is hardcoded, so this value at list will be 1 right?

💬 laanwj commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2761131196)
Oh! Nice detective work. Didn't really consider that, yes it's definitely possible for routing tables to be larger than 4k. There should probably be *some* limit to avoid crash/OOM in case of unexpected OS behavior but it doesn't need to be that low.

> do you have any approach-inclination between either i) querying a dummy route vs ii) the current query (NLM_F_DUMP) with a multi-part response handler + a filter for default routes?

i have a slight preference for getting all information then sif
...

👍 laanwj approved a pull request: "contrib: document asmap-tool commands more thoroughly"
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2725282806)
re-ACK 6afffba34e086de4cf0bb86729e12d116c1dcc9b