💬 paplorinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722725094)
If we need to document the behavior of a method, it suggests that we’ve acknowledged it may not be intuitive for everyone, indicating that we’ve conceded on finding a better design.
I think there's a way we can avoid the comment: by separating the two concerns of getting a value from cache and checking if it's spent or not.
Or if you insist, we can call it "GetUnspent" in which case we can also get rid of the comment and call-site asserts and keep the current behavior.
What do you think?
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722725094)
If we need to document the behavior of a method, it suggests that we’ve acknowledged it may not be intuitive for everyone, indicating that we’ve conceded on finding a better design.
I think there's a way we can avoid the comment: by separating the two concerns of getting a value from cache and checking if it's spent or not.
Or if you insist, we can call it "GetUnspent" in which case we can also get rid of the comment and call-site asserts and keep the current behavior.
What do you think?
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1722725975)
Thanks, added back
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1722725975)
Thanks, added back
💬 maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2298040795)
rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2298040795)
rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2298062851)
Would be great to get the final issues resolved. Possible way forward, preserving backwards compatibility:
<details>
<summary>
diff
</summary>
```diff
diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
index ff5a7ebd00..e83b5f38b6 100644
--- a/src/node/chainstatemanager_args.cpp
+++ b/src/node/chainstatemanager_args.cpp
@@ -33,7 +33,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
if (auto value{args.
...
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2298062851)
Would be great to get the final issues resolved. Possible way forward, preserving backwards compatibility:
<details>
<summary>
diff
</summary>
```diff
diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
index ff5a7ebd00..e83b5f38b6 100644
--- a/src/node/chainstatemanager_args.cpp
+++ b/src/node/chainstatemanager_args.cpp
@@ -33,7 +33,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
if (auto value{args.
...
⚠️ MummaStacks opened an issue: "Running Bitcoin Bitcore on new Apple M3"
(https://github.com/bitcoin/bitcoin/issues/30680)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
“Bitcoin Core” can’t be opened because Apple cannot check it for malicious software. This software needs to be updated. Contact the developer for more information."
### Expected behaviour
upload lates version of the application
### Steps to reproduce
wont allow me to install application
### Relevant log output
_No response_
### How did you obtain Bitcoin Core
Compiled from source
...
(https://github.com/bitcoin/bitcoin/issues/30680)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
“Bitcoin Core” can’t be opened because Apple cannot check it for malicious software. This software needs to be updated. Contact the developer for more information."
### Expected behaviour
upload lates version of the application
### Steps to reproduce
wont allow me to install application
### Relevant log output
_No response_
### How did you obtain Bitcoin Core
Compiled from source
...
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2298084825)
Edited pull description and title to remove the mention of the global, now that it is removed in the last commit
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2298084825)
Edited pull description and title to remove the mention of the global, now that it is removed in the last commit
💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2298086275)
Is this rfm with three acks, or does it need more, or is it waiting for after the branch off?
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2298086275)
Is this rfm with three acks, or does it need more, or is it waiting for after the branch off?
💬 maflcko commented on pull request "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2298095817)
You'll have to run the test locally, after you modify it and before you push the code to a pull request. There is a contributing guide for developers and how to run the tests.
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2298095817)
You'll have to run the test locally, after you modify it and before you push the code to a pull request. There is a contributing guide for developers and how to run the tests.
👍 TheCharlatan approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2247125663)
Re-ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2247125663)
Re-ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff
💬 maflcko commented on issue "Unable to build Bitcoin using './contrib/guix/guix-build'":
(https://github.com/bitcoin/bitcoin/issues/30676#issuecomment-2298116470)
There is a packaging error fixed in https://salsa.debian.org/debian/guix/-/commit/b27fcc3fa6d3633eb2beceb014507045f534f2d0
You can ask for it to be backported upstream, or work around it manually for now via `apt install netbase`
(https://github.com/bitcoin/bitcoin/issues/30676#issuecomment-2298116470)
There is a packaging error fixed in https://salsa.debian.org/debian/guix/-/commit/b27fcc3fa6d3633eb2beceb014507045f534f2d0
You can ask for it to be backported upstream, or work around it manually for now via `apt install netbase`
💬 maflcko commented on issue "Running Bitcoin Bitcore on new Apple M3":
(https://github.com/bitcoin/bitcoin/issues/30680#issuecomment-2298119594)
This may be a duplicate of #15774 ?
(https://github.com/bitcoin/bitcoin/issues/30680#issuecomment-2298119594)
This may be a duplicate of #15774 ?
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2298177522)
Apologies for another push.
Noticed that some of the commit messages were incorrect/imprecise:
"Only touching functions that will be modified in upcoming 2 commits." -> "Only touching functions that will be modified in next commit."
(A prior local version had code fixups happening after the de-Hungarianization and vector->span, which wasn't as smooth).
"Lines will be touched in upcoming commits within this PR." -> "Lines will be touched in next 2 commits."
Content change: The de-Hungari
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2298177522)
Apologies for another push.
Noticed that some of the commit messages were incorrect/imprecise:
"Only touching functions that will be modified in upcoming 2 commits." -> "Only touching functions that will be modified in next commit."
(A prior local version had code fixups happening after the de-Hungarianization and vector->span, which wasn't as smooth).
"Lines will be touched in upcoming commits within this PR." -> "Lines will be touched in next 2 commits."
Content change: The de-Hungari
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722846451)
style nit in 8ad548f389f6abd16db39dadafe3fbeefaedec3a: (up to you) If you want, you can add the missing `{}` after all `if`, according to the dev notes, in this commit. Feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722846451)
style nit in 8ad548f389f6abd16db39dadafe3fbeefaedec3a: (up to you) If you want, you can add the missing `{}` after all `if`, according to the dev notes, in this commit. Feel free to ignore.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722843198)
> Content change: The de-Hungarianization commit was adding a space after `if`, which now happens in the commit before.
style nit in 08a880de6c94bc84c1f43a3845e1645d3eb67607: Not sure about adding missing space to lines in one commit and then modifying the lines again in a rename commit. I think adding "missing" space can be done in the rename commit. (Same for the other clang-format changes in "unrelated lines" in commit 08a880de6c94bc84c1f43a3845e1645d3eb67607) (But just a style nit)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722843198)
> Content change: The de-Hungarianization commit was adding a space after `if`, which now happens in the commit before.
style nit in 08a880de6c94bc84c1f43a3845e1645d3eb67607: Not sure about adding missing space to lines in one commit and then modifying the lines again in a rename commit. I think adding "missing" space can be done in the rename commit. (Same for the other clang-format changes in "unrelated lines" in commit 08a880de6c94bc84c1f43a3845e1645d3eb67607) (But just a style nit)
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722852311)
style nit in 2c68649f0932dac146b941fb6b4e2fbc52fbd8a0: Not sure about switching string/string_views to a raw C-array in C++ code when there is no reason for it. Modern compilers can optimize away sting/string_view sizes on literals, so my preference would be to not change the type here.
Example:
```cpp
int main() {
std::string sv{"04678afdb0"};
return sv.size();
}
```
Optimized into:
```cpp
int main() {
return 10;
}
```
on modern compilers.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722852311)
style nit in 2c68649f0932dac146b941fb6b4e2fbc52fbd8a0: Not sure about switching string/string_views to a raw C-array in C++ code when there is no reason for it. Modern compilers can optimize away sting/string_view sizes on literals, so my preference would be to not change the type here.
Example:
```cpp
int main() {
std::string sv{"04678afdb0"};
return sv.size();
}
```
Optimized into:
```cpp
int main() {
return 10;
}
```
on modern compilers.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722867989)
> I don't think...
>
> ```c++
> s = ToScript(HexLiteral("0302ff030302ff03"));
> ```
>
> ...will be too bad.
Sure, up to you, it is just a style nit :)
I still think `s = ScriptFromHex("0302ff030302ff03")` is better, because it is shorter, more consistent in the test case, and comes with the same checks at roughly the same CPU cost.
If you really want to change it, maybe it can be done in the follow-up that allows `std::byte` serialization into script? This would also avoid havi
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722867989)
> I don't think...
>
> ```c++
> s = ToScript(HexLiteral("0302ff030302ff03"));
> ```
>
> ...will be too bad.
Sure, up to you, it is just a style nit :)
I still think `s = ScriptFromHex("0302ff030302ff03")` is better, because it is shorter, more consistent in the test case, and comes with the same checks at roughly the same CPU cost.
If you really want to change it, maybe it can be done in the follow-up that allows `std::byte` serialization into script? This would also avoid havi
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722890964)
The commit is modifying the line just above the if, and then the second commit modifies both (and it keeps the second commit a pure rename).
Is the concern that devs running `git blame` will have to exclude multiple commits?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722890964)
The commit is modifying the line just above the if, and then the second commit modifies both (and it keeps the second commit a pure rename).
Is the concern that devs running `git blame` will have to exclude multiple commits?
💬 maflcko commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2298267060)
Are you still working on this? There is outstanding review feedback, asking if this is a breaking change in the verification steps? https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2125882826
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2298267060)
Are you still working on this? There is outstanding review feedback, asking if this is a breaking change in the verification steps? https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2125882826
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722903409)
Aha, hadn't fully internalized that braces were only allowed to be skip if the then-statement appeared on the same line.
Will fix here and possibly other places if I retouch.
Guess you prefer not hoisting up the `return false;` onto the same line since it disrupts `git blame`?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722903409)
Aha, hadn't fully internalized that braces were only allowed to be skip if the then-statement appeared on the same line.
Will fix here and possibly other places if I retouch.
Guess you prefer not hoisting up the `return false;` onto the same line since it disrupts `git blame`?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722911225)
> Is the concern that devs running `git blame` will have to exclude multiple commits?
Yes. Repeatedly modifying the same lines of code can not be avoided sometimes, but seems better to minimize, because:
* (as you say) It increases the `git blame` depth, and decreases the usefulness of the `git log -S` parity search (or similar tools)
* Reviewers will have to look at the same line twice, which may take more time
Obviously this conflicts with the desire to keep unrelated changes in sep
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722911225)
> Is the concern that devs running `git blame` will have to exclude multiple commits?
Yes. Repeatedly modifying the same lines of code can not be avoided sometimes, but seems better to minimize, because:
* (as you say) It increases the `git blame` depth, and decreases the usefulness of the `git log -S` parity search (or similar tools)
* Reviewers will have to look at the same line twice, which may take more time
Obviously this conflicts with the desire to keep unrelated changes in sep
...