💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615734654)
The previous implementation is 10^-5, and the newer one is 10^-6.
IMO, it's worth creating it a constant with an expressive name to make reading this piece easier.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615734654)
The previous implementation is 10^-5, and the newer one is 10^-6.
IMO, it's worth creating it a constant with an expressive name to make reading this piece easier.
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615711645)
As per the code in `_bulk_tx`, the actual weight is indeed off by 3 WUs, consider getting rid of `might be` here, it adds a bit of uncertainty imo.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615711645)
As per the code in `_bulk_tx`, the actual weight is indeed off by 3 WUs, consider getting rid of `might be` here, it adds a bit of uncertainty imo.
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615710140)
Should there also be an assertion disallowing passing both `target_weight` and `fee`?
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615710140)
Should there also be an assertion disallowing passing both `target_weight` and `fee`?
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615736403)
Preference style: Although this is not a fresh test, but IMHO it makes it far easier for the reviewer to read when the variable name contains the unit as well such as `child_feerate_satvb` or `child_fee_sat`.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615736403)
Preference style: Although this is not a fresh test, but IMHO it makes it far easier for the reviewer to read when the variable name contains the unit as well such as `child_feerate_satvb` or `child_fee_sat`.
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615739344)
Nice catch here that fixes the calculation!
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1615739344)
Nice catch here that fixes the calculation!
⚠️ carnhofdaki opened an issue: "bitcoin-cli hanging on RPC in an empty datadir"
(https://github.com/bitcoin/bitcoin/issues/30181)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
`bitcoin-cli` is hanging indefinitely when called with `stop` in an empty datadir.
### Expected behaviour
I would prefer it to exit with error like it does when the `datadir` directory is non-existent.
```sh
$ rmdir empty
$ bitcoin-cli -datadir=empty stop
Error: Specified data directory "empty" does not exist.
```
### Steps to reproduce
Simple shell example:
```sh
$ mkdir em
...
(https://github.com/bitcoin/bitcoin/issues/30181)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
`bitcoin-cli` is hanging indefinitely when called with `stop` in an empty datadir.
### Expected behaviour
I would prefer it to exit with error like it does when the `datadir` directory is non-existent.
```sh
$ rmdir empty
$ bitcoin-cli -datadir=empty stop
Error: Specified data directory "empty" does not exist.
```
### Steps to reproduce
Simple shell example:
```sh
$ mkdir em
...
✅ carnhofdaki closed an issue: "bitcoin-cli hanging on RPC in an empty datadir"
(https://github.com/bitcoin/bitcoin/issues/30181)
(https://github.com/bitcoin/bitcoin/issues/30181)
💬 carnhofdaki commented on issue "bitcoin-cli hanging on RPC in an empty datadir":
(https://github.com/bitcoin/bitcoin/issues/30181#issuecomment-2133051810)
Closing. Seems it was my own local issue.
Can not be reproduced with v27.0 release.
(https://github.com/bitcoin/bitcoin/issues/30181#issuecomment-2133051810)
Closing. Seems it was my own local issue.
Can not be reproduced with v27.0 release.
🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2080398804)
Finished the rest of my code review and tested 97ae5d4eefe4 again on Intel macOS 14.5, Ubuntu 24.04 and Windows.
I noticed that the renewal for IPv6 fails (at least on Ubuntu and macOS) with `NO_RESOURCES`. IPv4 renewal works fine. Ten minutes later it tries again and the mapping succeeds.
IIUC OPNsense uses miniupnpd 2.3.1, which is two years behind but [at first glance](https://github.com/miniupnp/miniupnp/compare/miniupnpd_2_3_1...miniupnpd_2_3_6) I don't see any recent fixes related t
...
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2080398804)
Finished the rest of my code review and tested 97ae5d4eefe4 again on Intel macOS 14.5, Ubuntu 24.04 and Windows.
I noticed that the renewal for IPv6 fails (at least on Ubuntu and macOS) with `NO_RESOURCES`. IPv4 renewal works fine. Ten minutes later it tries again and the mapping succeeds.
IIUC OPNsense uses miniupnpd 2.3.1, which is two years behind but [at first glance](https://github.com/miniupnp/miniupnp/compare/miniupnpd_2_3_1...miniupnpd_2_3_6) I don't see any recent fixes related t
...
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1615728471)
bfde83050ece1f617efdb4098b5efeeb1a08b65e nit: `UPnP, PCP or NAT-PMP`
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1615728471)
bfde83050ece1f617efdb4098b5efeeb1a08b65e nit: `UPnP, PCP or NAT-PMP`
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1615868425)
Sure, ok, really i'm just considering NAT-PMP to be PCPv0.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1615868425)
Sure, ok, really i'm just considering NAT-PMP to be PCPv0.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133203743)
> I noticed that the renewal for IPv6 fails (at least on Ubuntu and macOS) with NO_RESOURCES. IPv4 renewal works fine. Ten minutes later it tries again and the mapping succeeds.
Is this after re-launching bitcoind? Mind that restarting will roll a new mapping nonce, which means that from the point of your router it's a new application trying to get the port, which will prevent it from making the mapping until the old mapping expires. But that should go away after one cycle and not happen agai
...
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133203743)
> I noticed that the renewal for IPv6 fails (at least on Ubuntu and macOS) with NO_RESOURCES. IPv4 renewal works fine. Ten minutes later it tries again and the mapping succeeds.
Is this after re-launching bitcoind? Mind that restarting will roll a new mapping nonce, which means that from the point of your router it's a new application trying to get the port, which will prevent it from making the mapping until the old mapping expires. But that should go away after one cycle and not happen agai
...
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133212294)
> Is this after re-launching bitcoind?
No, I hadn't run the node in a while. At startup it opens the port, only renewal fails. Leaving the node running, this seems to happen half the time, as you would expect: port is opened, renewal fails, mapping expires, so opening succeeds again, renewal fails, etc.
> Were you building with libnatpmp before?
No, I had that disabled in the past. I'll see if I can trigger it again with more detailed logs. But you're right, it might be an existing is
...
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133212294)
> Is this after re-launching bitcoind?
No, I hadn't run the node in a while. At startup it opens the port, only renewal fails. Leaving the node running, this seems to happen half the time, as you would expect: port is opened, renewal fails, mapping expires, so opening succeeds again, renewal fails, etc.
> Were you building with libnatpmp before?
No, I had that disabled in the past. I'll see if I can trigger it again with more detailed logs. But you're right, it might be an existing is
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133233029)
> No, I hadn't run the node in a while. At startup it opens the port, only renewal fails. Leaving the node running, this seems to happen half the time, as you would expect: port is opened, renewal fails, mapping expires, so opening succeeds again, renewal fails, etc.
This is the opposite from what's expected.
- But it only happens for IPv6, not IPv4. i wonder if that's because IPv4 is the first mapping created? Maybe it doesn't like the same nonce being used for multiple different mappings
...
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133233029)
> No, I hadn't run the node in a while. At startup it opens the port, only renewal fails. Leaving the node running, this seems to happen half the time, as you would expect: port is opened, renewal fails, mapping expires, so opening succeeds again, renewal fails, etc.
This is the opposite from what's expected.
- But it only happens for IPv6, not IPv4. i wonder if that's because IPv4 is the first mapping created? Maybe it doesn't like the same nonce being used for multiple different mappings
...
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133257542)
On macOS I recompiled with `PORT_MAPPING_REANNOUNCE_PERIOD` set to 5 minutes and IPv4 commented out. At startup it adds three mappings. After a few minutes at renewal it complains with `NO_RESOURCES`. Trying some other things...
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133257542)
On macOS I recompiled with `PORT_MAPPING_REANNOUNCE_PERIOD` set to 5 minutes and IPv4 commented out. At startup it adds three mappings. After a few minutes at renewal it complains with `NO_RESOURCES`. Trying some other things...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133264508)
> After a few minutes at renewal it complains with NO_RESOURCES.
Does it complain for all three mappings?
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133264508)
> After a few minutes at renewal it complains with NO_RESOURCES.
Does it complain for all three mappings?
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133270608)
Yes, for all three.
I then recompiled and set the renewal interval at 6/8 - 7/8. Started the node again (using a fresh port). Same pattern: three mappings succeeed, at renewal all three complain about NO_RESOURCES. (And then a few minutes later they succesfully get a mapping again).
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133270608)
Yes, for all three.
I then recompiled and set the renewal interval at 6/8 - 7/8. Started the node again (using a fresh port). Same pattern: three mappings succeeed, at renewal all three complain about NO_RESOURCES. (And then a few minutes later they succesfully get a mapping again).
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133279400)
Actually this might an existing bug: https://github.com/miniupnp/miniupnp/commit/7bd0877b8fd9a1c1c59cdf426b4640b3cee2bf61
I'll see if I can convince OPNsense to ship >= 2.3.6
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133279400)
Actually this might an existing bug: https://github.com/miniupnp/miniupnp/commit/7bd0877b8fd9a1c1c59cdf426b4640b3cee2bf61
I'll see if I can convince OPNsense to ship >= 2.3.6
👍 hebasto approved a pull request: "[27.x] Backports and rc1"
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2080735297)
ACK cb6def3855427b613357696ba16a431c7964dbcc.
Feel free to update translations with `python3 ../bitcoin-maintainer-tools/update-translations.py` or cherry-pick the https://github.com/hebasto/bitcoin/commit/c9c42cc5e9f5740c4700b18a28d809db53880db2 commit.
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2080735297)
ACK cb6def3855427b613357696ba16a431c7964dbcc.
Feel free to update translations with `python3 ../bitcoin-maintainer-tools/update-translations.py` or cherry-pick the https://github.com/hebasto/bitcoin/commit/c9c42cc5e9f5740c4700b18a28d809db53880db2 commit.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133309242)
That does look like an issue that would affect renewal, but i'm not sure it would cause this specific issue, if i understand correctly it'd bump the renewal timestamp so far it would live forever. It wouln't make it complain about the port being used. Unless it's an old mapping that sticks around. But that wouldn'e explain why it only happens on renewal.
As for generating a new nonce every time, this is what i meant:
```diff
diff --git a/src/mapport.cpp b/src/mapport.cpp
index e28ed52a58f3
...
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133309242)
That does look like an issue that would affect renewal, but i'm not sure it would cause this specific issue, if i understand correctly it'd bump the renewal timestamp so far it would live forever. It wouln't make it complain about the port being used. Unless it's an old mapping that sticks around. But that wouldn'e explain why it only happens on renewal.
As for generating a new nonce every time, this is what i meant:
```diff
diff --git a/src/mapport.cpp b/src/mapport.cpp
index e28ed52a58f3
...