💬 l0rinc commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243190113)
My understanding is that **Translation Memory Fillups** feature is `Growth plan` feature only.
I have tried uploading the English translation without any ids (called keys in Transifex)
<img src="https://github.com/user-attachments/assets/5c3e560c-1ad5-4009-baf9-48ab13f0723b" />
adding back a single id results in only that value being imported:
<img src="https://github.com/user-attachments/assets/04dd3da9-9a5f-4176-ad50-685edd4593f7" />
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243190113)
My understanding is that **Translation Memory Fillups** feature is `Growth plan` feature only.
I have tried uploading the English translation without any ids (called keys in Transifex)
<img src="https://github.com/user-attachments/assets/5c3e560c-1ad5-4009-baf9-48ab13f0723b" />
adding back a single id results in only that value being imported:
<img src="https://github.com/user-attachments/assets/04dd3da9-9a5f-4176-ad50-685edd4593f7" />
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314529708)
Indeed but master is even more smelly because settxfee is not tx fee but fee rate.
See discussion https://github.com/bitcoin/bitcoin/pull/31278
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314529708)
Indeed but master is even more smelly because settxfee is not tx fee but fee rate.
See discussion https://github.com/bitcoin/bitcoin/pull/31278
👍 stickies-v approved a pull request: "build: Set AUTHOR_WARNING on warnings"
(https://github.com/bitcoin/bitcoin/pull/33144#pullrequestreview-3174447010)
ACK fa6497ba71e9573d341c1c051af09b3ec2fc8d74
It makes sense to have an option to have configuration warnings dealt as errors, and it's unfortunate that the most elegant way to do that is by somewhat abusing `AUTHOR_WARNING`, but this seems like the least bad solution to me, until CMake has a `-Werror=all` option.
---
I've also been thinking about an alternative approach. `configure_warnings` is currently used for:
1. missing Python interpreter, which is necessary (without build failure
...
(https://github.com/bitcoin/bitcoin/pull/33144#pullrequestreview-3174447010)
ACK fa6497ba71e9573d341c1c051af09b3ec2fc8d74
It makes sense to have an option to have configuration warnings dealt as errors, and it's unfortunate that the most elegant way to do that is by somewhat abusing `AUTHOR_WARNING`, but this seems like the least bad solution to me, until CMake has a `-Werror=all` option.
---
I've also been thinking about an alternative approach. `configure_warnings` is currently used for:
1. missing Python interpreter, which is necessary (without build failure
...
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314535030)
Hmm, these are different options that can be set for various use cases see comment
https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915276592 and https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919611531
Not exactly sure which relation do you want a check for because there is already check if bounds for each individual option.
Perhaps suggest code ?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314535030)
Hmm, these are different options that can be set for various use cases see comment
https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915276592 and https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919611531
Not exactly sure which relation do you want a check for because there is already check if bounds for each individual option.
Perhaps suggest code ?
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544616)
Yeah, done.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544616)
Yeah, done.
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544789)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544789)
Thanks, fixed.
💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314549905)
> Are you suggesting that I add a test case that checks that we dont create transaction when the tx fee as a result of the fee rate provided by settxfee is more than maxtxfee?
I’d be surprised if we don’t already have a test for that.
> Note: This wallet RPC is likely going to be removed soon.
"soon" is very relative here.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314549905)
> Are you suggesting that I add a test case that checks that we dont create transaction when the tx fee as a result of the fee rate provided by settxfee is more than maxtxfee?
I’d be surprised if we don’t already have a test for that.
> Note: This wallet RPC is likely going to be removed soon.
"soon" is very relative here.
🤔 l0rinc reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3174344920)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3174344920)
Concept ACK
💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314454304)
not sure I understand why this was needed
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314454304)
not sure I understand why this was needed
💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314448739)
could we fix this in https://github.com/bitcoin-core/leveldb-subtree/pulls instead?
I have opened https://github.com/bitcoin-core/leveldb-subtree/pull/56 (I'm also fine with doing it in a different PR, just wanted to give the possibility to solve it in a cleaner way)
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314448739)
could we fix this in https://github.com/bitcoin-core/leveldb-subtree/pulls instead?
I have opened https://github.com/bitcoin-core/leveldb-subtree/pull/56 (I'm also fine with doing it in a different PR, just wanted to give the possibility to solve it in a cleaner way)
💬 l0rinc commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2314579166)
I didn't mean my response to be offensive before, and while still not sure I fully follow the arguments, I agree that stabilizing the formatting and unifying class/struct braces are separate concerns, so I have removed the unification, only kept the change making formatters explicit.
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2314579166)
I didn't mean my response to be offensive before, and while still not sure I fully follow the arguments, I agree that stabilizing the formatting and unifying class/struct braces are separate concerns, so I have removed the unification, only kept the change making formatters explicit.
💬 l0rinc commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#issuecomment-3243270669)
Kept the formatter config only without the class/struct brace unification, it's ready for re-review, thanks for the comments.
(https://github.com/bitcoin/bitcoin/pull/32813#issuecomment-3243270669)
Kept the formatter config only without the class/struct brace unification, it's ready for re-review, thanks for the comments.
💬 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
```