💬 hodlinator commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3243290826)
Tested but did not dig deep into the code for now. Spent more than a day adding debug code in Bitcoin Core and fighting with my OpenWrt router (https://github.com/openwrt/packages/issues/17871#issuecomment-3243222244). Now that the router has been fully reset without any lingering configs from prior major versions, PCP & NATPMP is actually working.
On master (in 7/7 attempts roughly):
```
₿ cmake --build build -j16 --target=bitcoind && ./build/bin/bitcoind -regtest -debug=net
...
Bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3243290826)
Tested but did not dig deep into the code for now. Spent more than a day adding debug code in Bitcoin Core and fighting with my OpenWrt router (https://github.com/openwrt/packages/issues/17871#issuecomment-3243222244). Now that the router has been fully reset without any lingering configs from prior major versions, PCP & NATPMP is actually working.
On master (in 7/7 attempts roughly):
```
₿ cmake --build build -j16 --target=bitcoind && ./build/bin/bitcoind -regtest -debug=net
...
Bitcoin
...
⚠️ l0rinc opened an issue: "InitError() doesn't always halt node startup when blockchain state exists"
(https://github.com/bitcoin/bitcoin/issues/33276)
As described in https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305592114, when `InitError()` is triggered during initialization, the node prints the error but sometimes continues running, if blockchain data already exists in the datadir.
```bash
rm -rfd demo build && mkdir -p demo && cmake -B build && cmake --build build -j$(nproc)
# empty state fails with error
build/bin/bitcoind -datadir=demo -stopatheight=100 -i2psam=invalid_addr
# init state with 10 blocks
build/bin/bitcoind -da
...
(https://github.com/bitcoin/bitcoin/issues/33276)
As described in https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305592114, when `InitError()` is triggered during initialization, the node prints the error but sometimes continues running, if blockchain data already exists in the datadir.
```bash
rm -rfd demo build && mkdir -p demo && cmake -B build && cmake --build build -j$(nproc)
# empty state fails with error
build/bin/bitcoind -datadir=demo -stopatheight=100 -i2psam=invalid_addr
# init state with 10 blocks
build/bin/bitcoind -da
...
💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2314600353)
Opened https://github.com/bitcoin/bitcoin/issues/33276, please resolve this comment
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2314600353)
Opened https://github.com/bitcoin/bitcoin/issues/33276, please resolve this comment
💬 hodlinator commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3243304152)
> I don't see any pmp fallbacks happening in logs?
We only fall back to NATPMP for pcp_err == `UNSUPP_VERSION`:
https://github.com/bitcoin/bitcoin/blob/b2d07f872c58af9cfdf9f9a4af0645376f9b98cb/src/mapport.cpp#L77-L81
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3243304152)
> I don't see any pmp fallbacks happening in logs?
We only fall back to NATPMP for pcp_err == `UNSUPP_VERSION`:
https://github.com/bitcoin/bitcoin/blob/b2d07f872c58af9cfdf9f9a4af0645376f9b98cb/src/mapport.cpp#L77-L81
💬 achow101 commented on pull request "kernel: pre-29.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/31978#issuecomment-3243310067)
> Note that `du -h` produces 1024-based units, while `m_assumed_blockchain_size` and `m_assumed_chain_state_size` expect multiples of 10^9 bytes, which means adding 4.8% to `M` results, and 7.4% to `G` results.
No? We use multiples of 1024.
(https://github.com/bitcoin/bitcoin/pull/31978#issuecomment-3243310067)
> Note that `du -h` produces 1024-based units, while `m_assumed_blockchain_size` and `m_assumed_chain_state_size` expect multiples of 10^9 bytes, which means adding 4.8% to `M` results, and 7.4% to `G` results.
No? We use multiples of 1024.
💬 achow101 commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314606198)
Seems low, would expect 244.
```
$ du -csh --apparent-size testnet3/chainstate/ testnet3/blocks/
16G testnet3/chainstate/
205G testnet3/blocks/
221G total
```
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314606198)
Seems low, would expect 244.
```
$ du -csh --apparent-size testnet3/chainstate/ testnet3/blocks/
16G testnet3/chainstate/
205G testnet3/blocks/
221G total
```
💬 achow101 commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314607768)
Seems low, would expect 20.
```
du -csh --apparent-size signet/chainstate/ signet/blocks/
2.8G signet/chainstate/
15G signet/blocks/
18G total
```
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314607768)
Seems low, would expect 20.
```
du -csh --apparent-size signet/chainstate/ signet/blocks/
2.8G signet/chainstate/
15G signet/blocks/
18G total
```
💬 achow101 commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314607076)
I think testnet4 blockchain size is bigger
```
du -csh --apparent-size testnet4/chainstate/ testnet4/blocks/
928M testnet4/chainstate/
20G testnet4/blocks/
20G total
```
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2314607076)
I think testnet4 blockchain size is bigger
```
du -csh --apparent-size testnet4/chainstate/ testnet4/blocks/
928M testnet4/chainstate/
20G testnet4/blocks/
20G total
```
💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3243349872)
> Hmm, I forgot that `CachedTxIsFromMe` does not use `IsFromMe`. Might need to change that in this PR as well.
I've pulled in 2 commits from #27865 which drop `CachedTxIsFromMe` and replace it with an `IsFromMe` check which is stored on disk.
> In both places we effectively treat it as `is_mine = IsMine(outputs) || IsFromMe(inputs)`.
While that's true, moving `IsFromMe` into `IsMine` does change `IsMine`'s semantics in other places, so I'm not sure that that would necessarily be correct
...
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3243349872)
> Hmm, I forgot that `CachedTxIsFromMe` does not use `IsFromMe`. Might need to change that in this PR as well.
I've pulled in 2 commits from #27865 which drop `CachedTxIsFromMe` and replace it with an `IsFromMe` check which is stored on disk.
> In both places we effectively treat it as `is_mine = IsMine(outputs) || IsFromMe(inputs)`.
While that's true, moving `IsFromMe` into `IsMine` does change `IsMine`'s semantics in other places, so I'm not sure that that would necessarily be correct
...
💬 achow101 commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243356083)
The `id` attribute is required by the XLIFF 1.2 spec: https://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#trans-unit
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243356083)
The `id` attribute is required by the XLIFF 1.2 spec: https://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#trans-unit
💬 achow101 commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3243360930)
> Was "Translation Memory Fillup" enabled then?
Only if it is by default. I don't know where that setting is.
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3243360930)
> Was "Translation Memory Fillup" enabled then?
Only if it is by default. I don't know where that setting is.
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314667768)
Done
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314667768)
Done
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668014)
Hm, I am not sure on this one. It sounds like this is so unlikely that the overflow value would probably go without an implementation of how to use it until something that triggers this actually happens. E.g. even if something goes on that looks suspicious the next release may be a few months out so the attack could already be carried out. In that case users would need to resync again anyway if I’m not mistaken. So I would prefer to only add this if there is a clear path to implement the overflo
...
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668014)
Hm, I am not sure on this one. It sounds like this is so unlikely that the overflow value would probably go without an implementation of how to use it until something that triggers this actually happens. E.g. even if something goes on that looks suspicious the next release may be a few months out so the attack could already be carried out. In that case users would need to resync again anyway if I’m not mistaken. So I would prefer to only add this if there is a clear path to implement the overflo
...
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668306)
Right, this code is "belts-and-suspenders" since we should never end up here since the base index should rewind us in this case before entering `CustomAppend`. So I will turn this into an error and drop the hashkey query below since we aren't recovering from that anyway.
This isn't something that is added here so it could be a separate PR but I will add it here since it simplifies the code a bit.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668306)
Right, this code is "belts-and-suspenders" since we should never end up here since the base index should rewind us in this case before entering `CustomAppend`. So I will turn this into an error and drop the hashkey query below since we aren't recovering from that anyway.
This isn't something that is added here so it could be a separate PR but I will add it here since it simplifies the code a bit.
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668493)
> Adding {0} default initializers in the struct DBVal declaration seems sensible?
Done
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668493)
> Adding {0} default initializers in the struct DBVal declaration seems sensible?
Done
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314670363)
Ok, this basically means we are only turning the values into per-block that are most in danger of overflowing. I have done this and since we are not removing most of the members anymore, I squashed that commit which I hope should make review a bit easier now as well. At least overall the change is simpler with this, I think.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314670363)
Ok, this basically means we are only turning the values into per-block that are most in danger of overflowing. I have done this and since we are not removing most of the members anymore, I squashed that commit which I hope should make review a bit easier now as well. At least overall the change is simpler with this, I think.
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3243423289)
> When running this index over signet, I end up with ~700 55kB ldb files, and a single 8.8MB ldb file. That seems like something is probably suboptimal?
This doesn’t seem to be caused by this change afaict, my old coinstatsindex on signet has the same file structure. I have tried to find the cause for this but so far I can’t really say for sure where the difference to the other indexes lies. There does not seem to be any difference in configuration and I looked into additional batching db wri
...
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3243423289)
> When running this index over signet, I end up with ~700 55kB ldb files, and a single 8.8MB ldb file. That seems like something is probably suboptimal?
This doesn’t seem to be caused by this change afaict, my old coinstatsindex on signet has the same file structure. I have tried to find the cause for this but so far I can’t really say for sure where the difference to the other indexes lies. There does not seem to be any difference in configuration and I looked into additional batching db wri
...
💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314824654)
You only need 65 bits to handle a 1MB block full of 61B txs cycling 21M BTC, so could:
* add a single uint8_t that we'd use two bits from for each
* add two uint16_t's to count the number of times MAX_MONEY is exceeded
* use __int128 or arith_uint256 for those fields
Could also just cap the value:
```c++
static inline void cap_add(CAmount& v, CAmount x)
{
if (std::numeric_limits<CAmount>::max() - x < v) {
v = std::numeric_limits<CAmount>::max();
} else {
...
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314824654)
You only need 65 bits to handle a 1MB block full of 61B txs cycling 21M BTC, so could:
* add a single uint8_t that we'd use two bits from for each
* add two uint16_t's to count the number of times MAX_MONEY is exceeded
* use __int128 or arith_uint256 for those fields
Could also just cap the value:
```c++
static inline void cap_add(CAmount& v, CAmount x)
{
if (std::numeric_limits<CAmount>::max() - x < v) {
v = std::numeric_limits<CAmount>::max();
} else {
...
💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314870867)
Had only kept it for readability, but yes we can definately do without it
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314870867)
Had only kept it for readability, but yes we can definately do without it
💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314871367)
yes. thanks
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314871367)
yes. thanks