🤔 furszy reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2322913465)
ACK 5bcc578
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2322913465)
ACK 5bcc578
📝 itornaza opened a pull request: "refactor: Appropriate re-naming of MAX_OPCODE after tapscript"
(https://github.com/bitcoin/bitcoin/pull/30953)
In `src/script/script.h:216` we define `MAX_OPCODE` to denote the maximum opcode that can be used in script.
However, after the addition of the `OP_CHECKSIGADD` with taproot in the opcode set, the `MAX_OPCODE` naming looks somehow misleading.
Renaming the variable to `MAX_SCRIPT_OPCODE` clearly denotes that the variable is intended for and referring exclusively to script (and not tapscript).
(https://github.com/bitcoin/bitcoin/pull/30953)
In `src/script/script.h:216` we define `MAX_OPCODE` to denote the maximum opcode that can be used in script.
However, after the addition of the `OP_CHECKSIGADD` with taproot in the opcode set, the `MAX_OPCODE` naming looks somehow misleading.
Renaming the variable to `MAX_SCRIPT_OPCODE` clearly denotes that the variable is intended for and referring exclusively to script (and not tapscript).
💬 achow101 commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209)
In 00a82f45004a887c2b58ed8da3db9280e0ec8b5c "multiprocess: Add serialization code for BlockValidationState"
There are some places where a `BlockValidationState` is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a `BlockValidateState` were serialized then deserialized, these assertions would be hit. I don't know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209)
In 00a82f45004a887c2b58ed8da3db9280e0ec8b5c "multiprocess: Add serialization code for BlockValidationState"
There are some places where a `BlockValidationState` is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a `BlockValidateState` were serialized then deserialized, these assertions would be hit. I don't know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble
...
💬 achow101 commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747)
In 246fd7faae85e871ba78101f1dee8d795437b8f6 "multiprocess: Add serialization code for vector<char>"
Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747)
In 246fd7faae85e871ba78101f1dee8d795437b8f6 "multiprocess: Add serialization code for vector<char>"
Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
💬 achow101 commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488)
In 246fd7faae85e871ba78101f1dee8d795437b8f6 "multiprocess: Add serialization code for vector<char>"
This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488)
In 246fd7faae85e871ba78101f1dee8d795437b8f6 "multiprocess: Add serialization code for vector<char>"
This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
💬 fjahr commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1771952837)
And aside from only hitting this in the context of rescan + blocks missing this is just about showing the progress and not the actual functionality the user seeks. So it didn't feel important enough for me to use another category mainly because of that. Unless there are strong opinions against it I will keep this as is.
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1771952837)
And aside from only hitting this in the context of rescan + blocks missing this is just about showing the progress and not the actual functionality the user seeks. So it didn't feel important enough for me to use another category mainly because of that. Unless there are strong opinions against it I will keep this as is.
💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2369170805)
> You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)
I don’t deny it that you need only one low-latency connection to run a pretty effective tx spamming attack. Though, in my understanding, you’re poi
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2369170805)
> You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)
I don’t deny it that you need only one low-latency connection to run a pretty effective tx spamming attack. Though, in my understanding, you’re poi
...
💬 achow101 commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2369179390)
ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2369179390)
ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
🚀 achow101 merged a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409)
(https://github.com/bitcoin/bitcoin/pull/30409)
💬 achow101 commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2369211315)
ACK f20fe33e94c6752e5d2ed92511c0bf51a10716ee
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2369211315)
ACK f20fe33e94c6752e5d2ed92511c0bf51a10716ee
💬 fjahr commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369236572)
> Can you explain this a bit better?
Yeah, my statement was wrong. I think I managed to confuse myself with some wrongfully placed debug logging. I have fixed the description.
> > Well, the approach seems to be similar with pruning:
>
> Sure, if that is the case, the pull request description should mention it. Also, the error message could probably be updated, because it only mentions pruning, no AU.
Updated the error message and realized that he same issue exists in `rescanblockchai
...
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369236572)
> Can you explain this a bit better?
Yeah, my statement was wrong. I think I managed to confuse myself with some wrongfully placed debug logging. I have fixed the description.
> > Well, the approach seems to be similar with pruning:
>
> Sure, if that is the case, the pull request description should mention it. Also, the error message could probably be updated, because it only mentions pruning, no AU.
Updated the error message and realized that he same issue exists in `rescanblockchai
...
🚀 achow101 merged a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678)
(https://github.com/bitcoin/bitcoin/pull/30678)
💬 fjahr commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369292131)
@achow101 do you have thoughts on descriptor import/rescan behavior with missing blocks because of ongoing assumeutxo background sync?
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369292131)
@achow101 do you have thoughts on descriptor import/rescan behavior with missing blocks because of ongoing assumeutxo background sync?
🤔 theuni reviewed a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2323243428)
Concept ACK, and scripted-diff lgtm ACK.
Bikeshed: How about `bitcoin-build-config.h` for symmetry with `bitcoin-build-info.h`?
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2323243428)
Concept ACK, and scripted-diff lgtm ACK.
Bikeshed: How about `bitcoin-build-config.h` for symmetry with `bitcoin-build-info.h`?
🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2323147329)
I think I need to rebase this now that #30409 is merged, so I'll also work on improving comments here as suggested in recent reviews.
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2323147329)
I think I need to rebase this now that #30409 is merged, so I'll also work on improving comments here as suggested in recent reviews.
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771976019)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747
> In [246fd7f](https://github.com/bitcoin/bitcoin/commit/246fd7faae85e871ba78101f1dee8d795437b8f6) "multiprocess: Add serialization code for vector"
>
> Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
tl;dr: Adding Span constructors would not eliminate the need for this builder. They would just allow defining a reader that's as generic as this builder
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771976019)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747
> In [246fd7f](https://github.com/bitcoin/bitcoin/commit/246fd7faae85e871ba78101f1dee8d795437b8f6) "multiprocess: Add serialization code for vector"
>
> Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
tl;dr: Adding Span constructors would not eliminate the need for this builder. They would just allow defining a reader that's as generic as this builder
...
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1772043669)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209
> There are some places where a `BlockValidationState` is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a `BlockValidateState` were serialized then deserialized, these assertions would be hit. I don't know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble into later.
I guess a way to
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1772043669)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209
> There are some places where a `BlockValidationState` is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a `BlockValidateState` were serialized then deserialized, these assertions would be hit. I don't know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble into later.
I guess a way to
...
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771987455)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488
> This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
This is saying defining a `CustomReadField` function corresponding to this `CustomBuildField` function is unnecessary. Not that this `CustomBuildField` is unnecessary.
Specifically a `CustomReadField` function that converts a `::capnp::Data` value into a `vector<ch
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771987455)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488
> This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
This is saying defining a `CustomReadField` function corresponding to this `CustomBuildField` function is unnecessary. Not that this `CustomBuildField` is unnecessary.
Specifically a `CustomReadField` function that converts a `::capnp::Data` value into a `vector<ch
...
💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772164812)
Yes, I often see peers with no services, both new connections still setting up and longer-lasting ones, where getpeerinfo returns
```
"services": "0000000000000000",
"servicesnames": [
],
```
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772164812)
Yes, I often see peers with no services, both new connections still setting up and longer-lasting ones, where getpeerinfo returns
```
"services": "0000000000000000",
"servicesnames": [
],
```
💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772171550)
(`continue` could probably `break` here instead)
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772171550)
(`continue` could probably `break` here instead)