💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#issuecomment-2193679390)
Updated 5a629a60c4739b94383596f9d040502b7b2c8b69 -> db8c6fc2ca5e1ad99aeda741e3b453f1f7bf7955 ([`pr/gwlog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.2) -> [`pr/gwlog.3`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/gwlog.2..pr/gwlog.3)) fixing initialization order bug in last push
(https://github.com/bitcoin/bitcoin/pull/30343#issuecomment-2193679390)
Updated 5a629a60c4739b94383596f9d040502b7b2c8b69 -> db8c6fc2ca5e1ad99aeda741e3b453f1f7bf7955 ([`pr/gwlog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.2) -> [`pr/gwlog.3`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/gwlog.2..pr/gwlog.3)) fixing initialization order bug in last push
✅ fanquake closed an issue: "Issue:XPUN ACCOUNT P Title"
(https://github.com/bitcoin/bitcoin/issues/30346)
(https://github.com/bitcoin/bitcoin/issues/30346)
:lock: fanquake locked an issue: "Issue:XPUN ACCOUNT P Title"
(https://github.com/bitcoin/bitcoin/issues/30346)
(https://github.com/bitcoin/bitcoin/issues/30346)
💬 maflcko commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656474489)
Are you sure that `LogError` is the right level? It should only be used for fatal errors where the node needs to shut down, according to the docs. This seems like a warning here, as suggested above.
Some wallet code can probably use `Result` and pass errors up, possibly here as well. And the caller can decide whether to log as a warning or return over RPC (or something else).
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656474489)
Are you sure that `LogError` is the right level? It should only be used for fatal errors where the node needs to shut down, according to the docs. This seems like a warning here, as suggested above.
Some wallet code can probably use `Result` and pass errors up, possibly here as well. And the caller can decide whether to log as a warning or return over RPC (or something else).
🤔 stratospher reviewed a pull request: "cli: restrict multiple exclusive argument usage in bitcoin-cli"
(https://github.com/bitcoin/bitcoin/pull/30148#pullrequestreview-2141106606)
tested ACK d2e8611.
2 more suggestions:
1. include new `OptionsCategory` in the fuzz test too - https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/src/test/fuzz/system.cpp#L56
2. add functional test coverage for this error in `test/functional/interface_bitcoin_cli.py`
(https://github.com/bitcoin/bitcoin/pull/30148#pullrequestreview-2141106606)
tested ACK d2e8611.
2 more suggestions:
1. include new `OptionsCategory` in the fuzz test too - https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/src/test/fuzz/system.cpp#L56
2. add functional test coverage for this error in `test/functional/interface_bitcoin_cli.py`
💬 stratospher commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1654372307)
b382bf4: nit: s/Cli/CLI
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1654372307)
b382bf4: nit: s/Cli/CLI
💬 maflcko commented on pull request "Remove redundant `-datacarrier` option":
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2193911303)
Not sure what to do here. So far it doesn't look like there has been a single reviewer in support of this change, so maybe it can be closed for now? If need or support arises in the future, it would be trivial to pick up again, but until then I am not sure this needs to sit around in the `open` state.
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2193911303)
Not sure what to do here. So far it doesn't look like there has been a single reviewer in support of this change, so maybe it can be closed for now? If need or support arises in the future, it would be trivial to pick up again, but until then I am not sure this needs to sit around in the `open` state.
💬 maflcko commented on pull request "Remove redundant `-datacarrier` option":
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2193914025)
Again, I'd be happy to review this change, if the changes to `init.cpp` and `./test/` were dropped and this was a refactor.
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2193914025)
Again, I'd be happy to review this change, if the changes to `init.cpp` and `./test/` were dropped and this was a refactor.
💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656555460)
> Are you sure that `LogError` is the right level? It should only be used for fatal errors where the node needs to shut down, according to the docs. This seems like a warning here, as suggested above.
Oh, that is interesting. The descriptions of those log levels in https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md are much more severe than I would have expected. Based on those descriptions I would expect LogError to be called LogFatal and
...
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656555460)
> Are you sure that `LogError` is the right level? It should only be used for fatal errors where the node needs to shut down, according to the docs. This seems like a warning here, as suggested above.
Oh, that is interesting. The descriptions of those log levels in https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md are much more severe than I would have expected. Based on those descriptions I would expect LogError to be called LogFatal and
...
✅ maflcko closed an issue: "Brainstorm: Transaction issuer-selected policy limits"
(https://github.com/bitcoin/bitcoin/issues/29454)
(https://github.com/bitcoin/bitcoin/issues/29454)
💬 maflcko commented on issue "Brainstorm: Transaction issuer-selected policy limits":
(https://github.com/bitcoin/bitcoin/issues/29454#issuecomment-2193933082)
Closing for now, due to inactivity. As this policy discussion isn't directly and exclusively related to the Bitcoin Core code base, it could make more sense to discuss with the greater ecosystem first, for example https://groups.google.com/g/bitcoindev, or https://delvingbitcoin.org/ ?
(https://github.com/bitcoin/bitcoin/issues/29454#issuecomment-2193933082)
Closing for now, due to inactivity. As this policy discussion isn't directly and exclusively related to the Bitcoin Core code base, it could make more sense to discuss with the greater ecosystem first, for example https://groups.google.com/g/bitcoindev, or https://delvingbitcoin.org/ ?
💬 Sjors commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1656567090)
Taken
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1656567090)
Taken
💬 Sjors commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1656567248)
Reworded
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1656567248)
Reworded
💬 Sjors commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#issuecomment-2193950235)
Addressed feedback and expanded PR description.
(https://github.com/bitcoin/bitcoin/pull/30335#issuecomment-2193950235)
Addressed feedback and expanded PR description.
💬 rkrux commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1656664613)
Interesting, I agree with this point. Returning the exact size of the transaction would make the RPC definition more robust.
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1656664613)
Interesting, I agree with this point. Returning the exact size of the transaction would make the RPC definition more robust.
💬 rkrux commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1656669632)
This commit (along with this^ suggestion) [b3ac117](https://github.com/bitcoin/bitcoin/pull/29523/commits/b3ac1179ff90fe261af4e6dc9c7af64e1518b435) can be a separate PR as well, doesn't seem necessary for this PR to be merged if I didn't miss anything.
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1656669632)
This commit (along with this^ suggestion) [b3ac117](https://github.com/bitcoin/bitcoin/pull/29523/commits/b3ac1179ff90fe261af4e6dc9c7af64e1518b435) can be a separate PR as well, doesn't seem necessary for this PR to be merged if I didn't miss anything.
💬 willcl-ark commented on issue "Add "maxuploadtargettimeframe" to change the timeframe considered by "maxuploadtarget"":
(https://github.com/bitcoin/bitcoin/issues/30176#issuecomment-2194078864)
Would it not be easier here to simply divide the monthly allowance by the number of days in the month, and use this as the `maxuploadtarget`?
Adding a redundant second setting vs just calculating a daily target doesn't make much sense to me. Even if we did add a monthly target (specified in days), then many months have different numbers of days in them, so this new handy setting will be wrong for many of them.
The best solution for you would seem to be to divide your monthly allowance by t
...
(https://github.com/bitcoin/bitcoin/issues/30176#issuecomment-2194078864)
Would it not be easier here to simply divide the monthly allowance by the number of days in the month, and use this as the `maxuploadtarget`?
Adding a redundant second setting vs just calculating a daily target doesn't make much sense to me. Even if we did add a monthly target (specified in days), then many months have different numbers of days in them, so this new handy setting will be wrong for many of them.
The best solution for you would seem to be to divide your monthly allowance by t
...
✅ maflcko closed an issue: "error cross compiling linux X64 => 32 bit i686"
(https://github.com/bitcoin/bitcoin/issues/30330)
(https://github.com/bitcoin/bitcoin/issues/30330)
💬 maflcko commented on issue "error cross compiling linux X64 => 32 bit i686":
(https://github.com/bitcoin/bitcoin/issues/30330#issuecomment-2194109016)
Closing for now, because this is not a Bitcoin Core issue, and solutions have been provided anyway.
(https://github.com/bitcoin/bitcoin/issues/30330#issuecomment-2194109016)
Closing for now, because this is not a Bitcoin Core issue, and solutions have been provided anyway.
💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2194112241)
Rearranging the order of the commits to make it easier to test the bugfix: the first test (53f714d8bfd2331651445bcadb773a10f4d30264) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix (65343ec49a6b73c4197dfc38e1c2f433b0a3838a).
I also updated the PR title and description to reflect that this PR is not a test-only PR anymore
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2194112241)
Rearranging the order of the commits to make it easier to test the bugfix: the first test (53f714d8bfd2331651445bcadb773a10f4d30264) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix (65343ec49a6b73c4197dfc38e1c2f433b0a3838a).
I also updated the PR title and description to reflect that this PR is not a test-only PR anymore