💬 willcl-ark commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1289659607)
@jonatack took the first. @pinheadmz removed the space. Thanks.
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1289659607)
@jonatack took the first. @pinheadmz removed the space. Thanks.
💬 willcl-ark commented on pull request "doc: Use GitHub's "Alert" markdown syntax":
(https://github.com/bitcoin/bitcoin/pull/28243#issuecomment-1672704282)
Thanks for the feedback, I share the worry with becoming locked in to a single vendor for our development.
IMO adding markdown syntax is not a big threat to this, but something is always more than nothing. FWIW Gitlab also [supports alerts via bootstrap](https://about.gitlab.com/handbook/markdown-guide/#colorful-sections) (so would require another change if we moved there ever), asciidoc has native admonition support, etc.
> They also make reading the source files more difficult, which is
...
(https://github.com/bitcoin/bitcoin/pull/28243#issuecomment-1672704282)
Thanks for the feedback, I share the worry with becoming locked in to a single vendor for our development.
IMO adding markdown syntax is not a big threat to this, but something is always more than nothing. FWIW Gitlab also [supports alerts via bootstrap](https://about.gitlab.com/handbook/markdown-guide/#colorful-sections) (so would require another change if we moved there ever), asciidoc has native admonition support, etc.
> They also make reading the source files more difficult, which is
...
💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1289727862)
In 21ac6b2932b0f5ea9da4c251c65f4b88dfe8e983 "test: wallet, verify migration doesn't crash for an invalid script"
s/former/latter
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1289727862)
In 21ac6b2932b0f5ea9da4c251c65f4b88dfe8e983 "test: wallet, verify migration doesn't crash for an invalid script"
s/former/latter
💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1289724128)
In 01126667b16991b11766bbd17a73bee2505ae56f "wallet: disallow migration of invalid or not-watched scripts"
Instead of iterating over `not_migrated_scripts` for every address book record that gets here, we could instead convert the address book destination to a script and just do a lookup in the set.
```suggestion
CScript addr_script = GetScriptForDestination(addr_pair.first);
if (!addr_script.empty() && not_migrated_scripts.count(script) > 0) {
...
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1289724128)
In 01126667b16991b11766bbd17a73bee2505ae56f "wallet: disallow migration of invalid or not-watched scripts"
Instead of iterating over `not_migrated_scripts` for every address book record that gets here, we could instead convert the address book destination to a script and just do a lookup in the set.
```suggestion
CScript addr_script = GetScriptForDestination(addr_pair.first);
if (!addr_script.empty() && not_migrated_scripts.count(script) > 0) {
...
🤔 stratospher reviewed a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1554883653)
tested ACK 1c7582e.
tested it using [this branch](https://github.com/stratospher/bitcoin/commits/more_fuzz_202306_bip324_ciphersuite) which uses random inputs from `FuzzedDataProvider` to check whether the ciphertext produced by this PR and the python BIP code produce the same output.
I also liked the simpler current approach for streaming API changes. the interfaces of the cipher look more efficient and cleaner compared to the previous implementations!
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1554883653)
tested ACK 1c7582e.
tested it using [this branch](https://github.com/stratospher/bitcoin/commits/more_fuzz_202306_bip324_ciphersuite) which uses random inputs from `FuzzedDataProvider` to check whether the ciphertext produced by this PR and the python BIP code produce the same output.
I also liked the simpler current approach for streaming API changes. the interfaces of the cipher look more efficient and cleaner compared to the previous implementations!
💬 stratospher commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1283327643)
990f0f8: aad in the BIP allows upto 4095 max bytes (max garbage length). is this a performance saver too?
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1283327643)
990f0f8: aad in the BIP allows upto 4095 max bytes (max garbage length). is this a performance saver too?
💬 stratospher commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1285005961)
(micro nit)
1c7582e: s/accomodate/accommodate
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1285005961)
(micro nit)
1c7582e: s/accomodate/accommodate
💬 stratospher commented on issue "fuzz: Invoking python interpreter from a C++ file":
(https://github.com/bitcoin/bitcoin/issues/22973#issuecomment-1672899107)
discussed in https://github.com/bitcoin/bitcoin/issues/23915
(https://github.com/bitcoin/bitcoin/issues/22973#issuecomment-1672899107)
discussed in https://github.com/bitcoin/bitcoin/issues/23915
✅ stratospher closed an issue: "fuzz: Invoking python interpreter from a C++ file"
(https://github.com/bitcoin/bitcoin/issues/22973)
(https://github.com/bitcoin/bitcoin/issues/22973)
👍 dergoegge approved a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1571484607)
Code review ACK abe8536192c9f2cd6ba9d0e083f23dec4d20841f
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1571484607)
Code review ACK abe8536192c9f2cd6ba9d0e083f23dec4d20841f
💬 fanquake commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1672921810)
We can track and deal with all remaining comments in one of the followup PRs.
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1672921810)
We can track and deal with all remaining comments in one of the followup PRs.
🚀 fanquake merged a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008)
(https://github.com/bitcoin/bitcoin/pull/28008)
💬 dergoegge commented on pull request "p2p: outbound network diversity improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1672930044)
The title "p2p: outbound network diversity improvements" makes it sound like you are improving on the logic for making outbound connections but really you are refactoring and adding additional logging. Would you mind making the title more accurate?
> Please see the commit messages for details.
The PR description is added to the merge commit, so in my opinion it makes sense to have at least a summary of what the PR does in the description.
From our [docs](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1672930044)
The title "p2p: outbound network diversity improvements" makes it sound like you are improving on the logic for making outbound connections but really you are refactoring and adding additional logging. Would you mind making the title more accurate?
> Please see the commit messages for details.
The PR description is added to the merge commit, so in my opinion it makes sense to have at least a summary of what the PR does in the description.
From our [docs](https://github.com/bitcoin/bitco
...
🤔 dergoegge reviewed a pull request: "fuzz: fix a couple incorrect assertions in the `coins_view` target"
(https://github.com/bitcoin/bitcoin/pull/28215#pullrequestreview-1571557702)
lgtm (only one small suggestion inline)
> See commits messages for details.
I'd prefer if you summarize what the PR does in the description. Mostly the fact that these assertions are only correct if we use the default `CCoinsView` but incorrect if we were using an actual backend impl.
(https://github.com/bitcoin/bitcoin/pull/28215#pullrequestreview-1571557702)
lgtm (only one small suggestion inline)
> See commits messages for details.
I'd prefer if you summarize what the PR does in the description. Mostly the fact that these assertions are only correct if we use the default `CCoinsView` but incorrect if we were using an actual backend impl.
💬 dergoegge commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1289900910)
```suggestion
// Note we can't assert that `coin_using_get_coin == coin_using_backend_get_coin` because the coin in
// the cache may have been modified but not yet flushed.
```
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1289900910)
```suggestion
// Note we can't assert that `coin_using_get_coin == coin_using_backend_get_coin` because the coin in
// the cache may have been modified but not yet flushed.
```
💬 glozow commented on pull request "policy: Ephemeral anchors":
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1672959244)
> @instagibbs pushed 1 commit.
> [faeed68](https://github.com/bitcoin/bitcoin/commit/faeed687e5cde5e32750d93818dd1d4add837f24) nice crash bro
hacked a quick-ish fix on top of this branch [here](https://github.com/glozow/bitcoin/tree/2023-08-10-fix-crash-bro). As discussed offline, the general problem is with calling `TrimToSize` in the middle of package validation (meaning something can go from already-in-mempool to evicted, or just-submitted to evicted) so a coin may be cached in `m_view
...
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1672959244)
> @instagibbs pushed 1 commit.
> [faeed68](https://github.com/bitcoin/bitcoin/commit/faeed687e5cde5e32750d93818dd1d4add837f24) nice crash bro
hacked a quick-ish fix on top of this branch [here](https://github.com/glozow/bitcoin/tree/2023-08-10-fix-crash-bro). As discussed offline, the general problem is with calling `TrimToSize` in the middle of package validation (meaning something can go from already-in-mempool to evicted, or just-submitted to evicted) so a coin may be cached in `m_view
...
👍 stickies-v approved a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1570159243)
ACK 7721ab9139013d70ef0f058754fabc5aaeda2246
The changes in `blockstorage.cpp` and `blockstorage.h` are strict improvements, increasing visibility in flushing failures. I found the change in `validation.cpp` much more difficult to review, given the wide variety of ways in which `FlushStateToDisk()` is used. I've found that:
- it improves behaviour in some ways. For example, if `FlushBlockFile()` fails and the `WriteBlockIndexDB()` call still succeeds, we can end up in a situation where we ex
...
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1570159243)
ACK 7721ab9139013d70ef0f058754fabc5aaeda2246
The changes in `blockstorage.cpp` and `blockstorage.h` are strict improvements, increasing visibility in flushing failures. I found the change in `validation.cpp` much more difficult to review, given the wide variety of ways in which `FlushStateToDisk()` is used. I've found that:
- it improves behaviour in some ways. For example, if `FlushBlockFile()` fails and the `WriteBlockIndexDB()` call still succeeds, we can end up in a situation where we ex
...
💬 stickies-v commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1288920603)
nit: can be declared closer to where it's used
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1288920603)
nit: can be declared closer to where it's used
💬 stickies-v commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1288925171)
nit: long line
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1288925171)
nit: long line
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1672973129)
> Anything left to do here, for a required CI-only pull request with review?
I believe this PR is ready to merge unless @fanquake has more comment regarding its description.
However, it requires adjusting "Actions permissions" and "Workflow permissions" in the repo in a similar way as it was done in https://github.com/bitcoin-core/secp256k1/pull/1389.
@achow101 Would you mind taking care of that, please?
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1672973129)
> Anything left to do here, for a required CI-only pull request with review?
I believe this PR is ready to merge unless @fanquake has more comment regarding its description.
However, it requires adjusting "Actions permissions" and "Workflow permissions" in the repo in a similar way as it was done in https://github.com/bitcoin-core/secp256k1/pull/1389.
@achow101 Would you mind taking care of that, please?