💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723318351)
just noting this moved comment is extremely verbose/old comment and can probably just have the last paragraph kept?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723318351)
just noting this moved comment is extremely verbose/old comment and can probably just have the last paragraph kept?
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1722273831)
unrelated to your move-only change, but if txid==wtxid, looks like we're doing two insertions anyways here
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1722273831)
unrelated to your move-only change, but if txid==wtxid, looks like we're doing two insertions anyways here
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723349184)
If the wrapper is named `HaveMoreWork`, maybe have `GetTxToReconsider` be called `GetMoreWork`? Otherwise they seem incongruous.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723349184)
If the wrapper is named `HaveMoreWork`, maybe have `GetTxToReconsider` be called `GetMoreWork`? Otherwise they seem incongruous.
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723275394)
65d773feb7384a6893ee7e6a7b24dfa60f2d8d39
commit message seems grammatically incorrect:
"[refactor] move valid and tx processing to TxDownload"
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723275394)
65d773feb7384a6893ee7e6a7b24dfa60f2d8d39
commit message seems grammatically incorrect:
"[refactor] move valid and tx processing to TxDownload"
💬 glozow commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1723369871)
Doesn't this just make the docs less generalizable? Why is that better?
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1723369871)
Doesn't this just make the docs less generalizable? Why is that better?
💬 glozow commented on pull request "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#discussion_r1723372481)
The point of this test is to check that `signrawtransactionwithkey` works. Replacing the call to `signrawtransactionwithkey` with a different signing function seems wrong.
(https://github.com/bitcoin/bitcoin/pull/30667#discussion_r1723372481)
The point of this test is to check that `signrawtransactionwithkey` works. Replacing the call to `signrawtransactionwithkey` with a different signing function seems wrong.
💬 ryanofsky commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2298947289)
> rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?
I haven't been merging anything just because I think we are in feature freeze https://github.com/bitcoin/bitcoin/issues/29891
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2298947289)
> rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?
I haven't been merging anything just because I think we are in feature freeze https://github.com/bitcoin/bitcoin/issues/29891
💬 brunoerg commented on pull request "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`":
(https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723380632)
> Is this covered in unit tests?
No, it doesn't cover any p2p iteration.
(https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723380632)
> Is this covered in unit tests?
No, it doesn't cover any p2p iteration.
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1723392149)
Thanks, done. This is a good idea to reduce the diff and make it easier to review.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1723392149)
Thanks, done. This is a good idea to reduce the diff and make it easier to review.
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1723397494)
Not sure. I understand that this will add compile-time checks, however I think the benefits in test code are limited, because:
* Tests are deterministic and execute fast (even in valgrind and other sanitizers), so any issues are found at roughly the same CPU cost.
* If I change it here, I open up the door to change it elsewhere in this diff, which will make it more verbose and harder to review.
* The resulting code is more verbose at little benefit
So I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1723397494)
Not sure. I understand that this will add compile-time checks, however I think the benefits in test code are limited, because:
* Tests are deterministic and execute fast (even in valgrind and other sanitizers), so any issues are found at roughly the same CPU cost.
* If I change it here, I open up the door to change it elsewhere in this diff, which will make it more verbose and harder to review.
* The resulting code is more verbose at little benefit
So I'll leave this as-is for now.
💬 brunoerg commented on pull request "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`":
(https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723404448)
Tbh, most part of the functional test relies on checking logs which I agree it's a bad way to test behavior. However, since we're just registering and forgetting peers, there is no other way to test stuff on the functional side, except disconnections.
I'll close this for now and suggest some improvements for this test according to the Erlay PRs.
(https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723404448)
Tbh, most part of the functional test relies on checking logs which I agree it's a bad way to test behavior. However, since we're just registering and forgetting peers, there is no other way to test stuff on the functional side, except disconnections.
I'll close this for now and suggest some improvements for this test according to the Erlay PRs.
✅ brunoerg closed a pull request: "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`"
(https://github.com/bitcoin/bitcoin/pull/30683)
(https://github.com/bitcoin/bitcoin/pull/30683)
💬 brunoerg commented on pull request "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`":
(https://github.com/bitcoin/bitcoin/pull/30683#issuecomment-2298981271)
https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723404448
(https://github.com/bitcoin/bitcoin/pull/30683#issuecomment-2298981271)
https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723404448
💬 paplorinc commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1723413368)
My take was that `~/bitcoin/src` is easier to generalize to e.g. `C:\\Users\\Username\\Documents\\bitcoin\\src` than `/path/to/project/root/src` to `C:\\Users\\Username\\Documents\\bitcoin\\src`, since there have to understand that `root` means `bitcoin` and `src` refers to the literal `src` folder.
-----
My inspiration for using concrete examples was https://github.com/hebasto/bitcoin/pull/328/files#diff-4d2a64ce14cb8b971dbba9455421b04ae7ed0c489c66d983664be5632b0de4a3R19
-----
If it
...
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1723413368)
My take was that `~/bitcoin/src` is easier to generalize to e.g. `C:\\Users\\Username\\Documents\\bitcoin\\src` than `/path/to/project/root/src` to `C:\\Users\\Username\\Documents\\bitcoin\\src`, since there have to understand that `root` means `bitcoin` and `src` refers to the literal `src` folder.
-----
My inspiration for using concrete examples was https://github.com/hebasto/bitcoin/pull/328/files#diff-4d2a64ce14cb8b971dbba9455421b04ae7ed0c489c66d983664be5632b0de4a3R19
-----
If it
...
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299002666)
> I can confirm that best block locator and last_processed_block being different is confusing
I wonder if something else could be done to resolve this confusion other than writing to every wallet file every time a new block is connected, even if the block doesn't contain any relevant transactions. To me, "last block processed" and "last block flushed" seem like different concepts, and we could force them to be the same but only if we:
- Give up flexibility of not needing to flush each wall
...
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299002666)
> I can confirm that best block locator and last_processed_block being different is confusing
I wonder if something else could be done to resolve this confusion other than writing to every wallet file every time a new block is connected, even if the block doesn't contain any relevant transactions. To me, "last block processed" and "last block flushed" seem like different concepts, and we could force them to be the same but only if we:
- Give up flexibility of not needing to flush each wall
...
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2299012568)
Force pushed to make the diff (minimally) smaller and easier to review. Should be trivial to re-review.
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2299012568)
Force pushed to make the diff (minimally) smaller and easier to review. Should be trivial to re-review.
📝 furszy opened a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684)
Currently, if users provide a non-boolean convertible value to a negated option,
such as '-nowallet=not_a_boolean', the initialization process results in a fatal
error, causing an unclean shutdown and displaying a poorly descriptive error
message:
"JSON value of type bool is not of expected type string."
This PR fixes the issue by checking the command-line argument type before
any code attempts to access it. Additionally, it improves the returned error
message, changing it to:
"Invalid
...
(https://github.com/bitcoin/bitcoin/pull/30684)
Currently, if users provide a non-boolean convertible value to a negated option,
such as '-nowallet=not_a_boolean', the initialization process results in a fatal
error, causing an unclean shutdown and displaying a poorly descriptive error
message:
"JSON value of type bool is not of expected type string."
This PR fixes the issue by checking the command-line argument type before
any code attempts to access it. Additionally, it improves the returned error
message, changing it to:
"Invalid
...
💬 fjahr commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299019789)
> I wonder if something else could be done to resolve this confusion
Ok, I would say it's a problem when users see behavior that is inconsistent because of this difference. If the users don't notice it I guess these don't have to be the same at all times. There could be a more "pull-based" approach where they are aligned when the user interacts with the wallet, similar to what I tried to do for the backup case in https://github.com/bitcoin/bitcoin/pull/30678. But I am not clear yet on which a
...
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299019789)
> I wonder if something else could be done to resolve this confusion
Ok, I would say it's a problem when users see behavior that is inconsistent because of this difference. If the users don't notice it I guess these don't have to be the same at all times. There could be a more "pull-based" approach where they are aligned when the user interacts with the wallet, similar to what I tried to do for the backup case in https://github.com/bitcoin/bitcoin/pull/30678. But I am not clear yet on which a
...
💬 glozow commented on pull request "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2299024211)
Thanks for your interest in contributing. I recommend reading the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) and [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md), which contain helpful tips for getting started, squashing commits and rebasing, motivation for refactoring, etc.
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2299024211)
Thanks for your interest in contributing. I recommend reading the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) and [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md), which contain helpful tips for getting started, squashing commits and rebasing, motivation for refactoring, etc.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2299042222)
> does not make the binary smaller (built x86_64-linux-gnu guix bitcoind binary) - actually grows by 0.04% (~49.6 KB).
Not sure if it's a dealbreaker of not, but it seems that we're not actually storing the hexadecimal values as an array of bytes, but rather as separate stack pushes, which might be the reason for the size increase, i.e.
```C++
HexLiteral("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f")
```
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2299042222)
> does not make the binary smaller (built x86_64-linux-gnu guix bitcoind binary) - actually grows by 0.04% (~49.6 KB).
Not sure if it's a dealbreaker of not, but it seems that we're not actually storing the hexadecimal values as an array of bytes, but rather as separate stack pushes, which might be the reason for the size increase, i.e.
```C++
HexLiteral("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f")
```
...