💬 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
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768153)
Moved in 9eff5601059
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768153)
Moved in 9eff5601059
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768252)
Added in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768252)
Added in 49d5bfdd7e7
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768303)
Thanks, updated in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768303)
Thanks, updated in 49d5bfdd7e7
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768365)
Oh no! fixed in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768365)
Oh no! fixed in 49d5bfdd7e7
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768428)
I agree on reflection, gave them new names in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768428)
I agree on reflection, gave them new names in 49d5bfdd7e7
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768473)
I've un-removed the comment in 9eff5601059, as I agree with @ryanofsky that not changing the existant behaviour is the "safer" default, and new behaviour can be introduced with the new option.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768473)
I've un-removed the comment in 9eff5601059, as I agree with @ryanofsky that not changing the existant behaviour is the "safer" default, and new behaviour can be introduced with the new option.
👍 rkrux approved a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2144759617)
tACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)
`make, test/functional` are successful.
Thanks for testing the filtering by node functionality, and replacing the comments with info/debug logs as suggested by @tdb3.
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2144759617)
tACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)
`make, test/functional` are successful.
Thanks for testing the filtering by node functionality, and replacing the comments with info/debug logs as suggested by @tdb3.
💬 alfonsoromanz commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2194199411)
Re-ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2194199411)
Re-ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
🤔 alfonsoromanz reviewed a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2144886875)
tACK 6809ffd8a6c589c515af48da2dd8367e4c6c2c26. I ran `test/functional/feature_coinstatsindex.py`, and the test passes.
+1 to informing the log as @tdb3 suggests.
You can now update your PR description to remove the note about the function reordering and the node restart.
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2144886875)
tACK 6809ffd8a6c589c515af48da2dd8367e4c6c2c26. I ran `test/functional/feature_coinstatsindex.py`, and the test passes.
+1 to informing the log as @tdb3 suggests.
You can now update your PR description to remove the note about the function reordering and the node restart.
💬 maflcko commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1656887109)
Yeah, seems fine to remove the recursion, given that real code probably doesn't evaluate the hash recursively.
Unrelated, I don't think `uint256` is generally guaranteed to be `uint64_t`-aligned. While probably not a problem here, the cast won't work in the general case:
```
bench/crypto_hash.cpp: runtime error: store to misaligned address 0x7ffe64f6caa1 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0x7ffe64f6caa1: note: pointer points here
00 00 00 00 00
...
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1656887109)
Yeah, seems fine to remove the recursion, given that real code probably doesn't evaluate the hash recursively.
Unrelated, I don't think `uint256` is generally guaranteed to be `uint64_t`-aligned. While probably not a problem here, the cast won't work in the general case:
```
bench/crypto_hash.cpp: runtime error: store to misaligned address 0x7ffe64f6caa1 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0x7ffe64f6caa1: note: pointer points here
00 00 00 00 00
...