💬 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
...
💬 sipa commented on issue ""bitcoin-cli" does not exist, while "bitcoind" does in ~/bitcoin/src folder":
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133324567)
`bitcoin-cli` is compiled by default, but without detailed steps to reproduce there is no way to know what lead to your situation.
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133324567)
`bitcoin-cli` is compiled by default, but without detailed steps to reproduce there is no way to know what lead to your situation.
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133341499)
> generating a new nonce every time for every mapping
That doesn't seem like a good idea. IIUC the nonce is used to recognise the requesting application, and to distinguish a renew request from some other app trying to use the same port.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133341499)
> generating a new nonce every time for every mapping
That doesn't seem like a good idea. IIUC the nonce is used to recognise the requesting application, and to distinguish a renew request from some other app trying to use the same port.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133347999)
> That doesn't seem like a good idea. IIUC the nonce is used to recognise the requesting application, and to distinguish a renew request from some other app trying to use the same port.
It's not supposed to be a good idea, but i wonder if it works around the issue with your router, if they implemented the protocol wrongly. i honestly have no idea what could be the problem here i'm just guessing.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133347999)
> That doesn't seem like a good idea. IIUC the nonce is used to recognise the requesting application, and to distinguish a renew request from some other app trying to use the same port.
It's not supposed to be a good idea, but i wonder if it works around the issue with your router, if they implemented the protocol wrongly. i honestly have no idea what could be the problem here i'm just guessing.
💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133351751)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133351751)
Concept ACK.
💬 maflcko commented on issue ""bitcoin-cli" does not exist, while "bitcoind" does in ~/bitcoin/src folder":
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133377482)
What is the output when you re-build the project? (Something like `make clean && make`)
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133377482)
What is the output when you re-build the project? (Something like `make clean && make`)
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133381489)
I thought perhaps the issue was that miniupnpd checks whether at least half the lease time went by and would refuse if not. But looking through the source code it doesn't appear to care about that. There's a few places that can trigger a `PCP_ERR_NO_RESOURCES` reply, but so far those don't offer an obvious explanation.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2133381489)
I thought perhaps the issue was that miniupnpd checks whether at least half the lease time went by and would refuse if not. But looking through the source code it doesn't appear to care about that. There's a few places that can trigger a `PCP_ERR_NO_RESOURCES` reply, but so far those don't offer an obvious explanation.
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1615998227)
RPC -> RFC (in a few other places too)
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1615998227)
RPC -> RFC (in a few other places too)
💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133434672)
> It's not a full drop-in replacement...
Then, perhaps, it's a good chance to avoid `size_t` for parameter and return types in the interface?
See: [Signed and Unsigned Types in Interfaces](https://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf)
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133434672)
> It's not a full drop-in replacement...
Then, perhaps, it's a good chance to avoid `size_t` for parameter and return types in the interface?
See: [Signed and Unsigned Types in Interfaces](https://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf)
📝 brunoerg opened a pull request: "rpc: net: follow-ups for #30062"
(https://github.com/bitcoin/bitcoin/pull/30183)
- Change `addrman` to reference to const since it isn't modified (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793).
- Improve documentation of `mapped_as`/`source_mapped_as` in `getrawaddrman` RPC by mentioning that both fields will be only available if asmap flag is set. It is the same message for `mapped_as` field in `getpeerinfo`.
(https://github.com/bitcoin/bitcoin/pull/30183)
- Change `addrman` to reference to const since it isn't modified (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793).
- Improve documentation of `mapped_as`/`source_mapped_as` in `getrawaddrman` RPC by mentioning that both fields will be only available if asmap flag is set. It is the same message for `mapped_as` field in `getpeerinfo`.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751)
Each byte in the output script counts as four weight units, so with the padding method used here, we can only ever increase the weight by multiples of four. If the weight of the unpadded tx is not a multiple of four already (i.e. tx.get_weight() % 4 != 0), it will also not be after the padding and the best we can do is to be not more off than 3 WUs in the worst case.
It would be nice if we could somehow do exact padding with weight unit precision, but to my knowledge there is no general way t
...
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751)
Each byte in the output script counts as four weight units, so with the padding method used here, we can only ever increase the weight by multiples of four. If the weight of the unpadded tx is not a multiple of four already (i.e. tx.get_weight() % 4 != 0), it will also not be after the padding and the best we can do is to be not more off than 3 WUs in the worst case.
It would be nice if we could somehow do exact padding with weight unit precision, but to my knowledge there is no general way t
...
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064960)
We need a running node to create a MiniWallet instance, so unfortunately doing these checks in an unit test wouldn't work. I considered putting it into an existing functional test but wouldn't know which one to pick, so creating a new MiniWallet-specific test where features can tested independently seemed to make the most sense.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064960)
We need a running node to create a MiniWallet instance, so unfortunately doing these checks in an unit test wouldn't work. I considered putting it into an existing functional test but wouldn't know which one to pick, so creating a new MiniWallet-specific test where features can tested independently seemed to make the most sense.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065138)
Passing `target_weight` with an absolute `fee` should still work fine. There is arguably less reason now to do so as passing a `fee_rate` is likely more convenient for most use-cases, but I don't think we have to disallow padding txs with an absolute fee.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065138)
Passing `target_weight` with an absolute `fee` should still work fine. There is arguably less reason now to do so as passing a `fee_rate` is likely more convenient for most use-cases, but I don't think we have to disallow padding txs with an absolute fee.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065536)
The actual weight can be larger by at most 3 WUs, [see comment above](https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751).
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065536)
The actual weight can be larger by at most 3 WUs, [see comment above](https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751).