💬 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
```
💬 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.