💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920748)
Good point. Removed this check.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920748)
Good point. Removed this check.
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920893)
Added a check
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920893)
Added a check
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920972)
Removed
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920972)
Removed
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921413)
Good point. I've added a check earlier that the number of records is even to avoid this kind of issue.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921413)
Good point. I've added a check earlier that the number of records is even to avoid this kind of issue.
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921595)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921595)
Done as suggested.
💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568092353)
Instead of hardcoding these new `rpcauth` lines, it might be better to expand the existing `users` list. This would apply to adjacent methods as well.
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568092353)
Instead of hardcoding these new `rpcauth` lines, it might be better to expand the existing `users` list. This would apply to adjacent methods as well.
💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568080142)
This seems to be using the argument `user` as either a string or a list depending on whether or not `password` was provided, which seems less straightforward than using a consistent type. Unless I'm missing something, the `password` argument might not be needed at all, since the `user` list contains the plaintext password. See:
https://github.com/bitcoin/bitcoin/blob/94836b3d96f7fe9ed4225bf7ada961ca5dd554e8/test/functional/rpc_whitelist.py#L40-L45
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568080142)
This seems to be using the argument `user` as either a string or a list depending on whether or not `password` was provided, which seems less straightforward than using a consistent type. Unless I'm missing something, the `password` argument might not be needed at all, since the `user` list contains the plaintext password. See:
https://github.com/bitcoin/bitcoin/blob/94836b3d96f7fe9ed4225bf7ada961ca5dd554e8/test/functional/rpc_whitelist.py#L40-L45
🤔 tdb3 reviewed a pull request: "Test/rpc whitelistdefault test"
(https://github.com/bitcoin/bitcoin/pull/29858#pullrequestreview-2004787892)
Thank you for picking this up. Enhancing testing for of the whitelist capability is important to notice regressions associated with a security-minded feature.
Concept ACK. Looks like there are opportunities to enhance the existing approach taken. Some inline comments were left.
Built and ran all functional tests (all passed).
Checked that the following comments were addressed from PR #17805.
- The additional tests appear to be integrated into rpc_whitelist.py
- Tests were organ
...
(https://github.com/bitcoin/bitcoin/pull/29858#pullrequestreview-2004787892)
Thank you for picking this up. Enhancing testing for of the whitelist capability is important to notice regressions associated with a security-minded feature.
Concept ACK. Looks like there are opportunities to enhance the existing approach taken. Some inline comments were left.
Built and ran all functional tests (all passed).
Checked that the following comments were addressed from PR #17805.
- The additional tests appear to be integrated into rpc_whitelist.py
- Tests were organ
...
💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568071896)
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
>Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568071896)
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
>Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.
💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568068748)
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
>If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts.
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568068748)
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
>If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts.
💬 laanwj commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060364162)
i'm divided on this conceptually, i think with the reputation it has now, another successful attack on xz specifically is unlikely (not more likely than any other library, at least). And i'm not sure we need to go out of our way to avoid it given how much of the FOSS ecosystem relies on it.
One thing i am slightly afraid of is that in avoiding xz we become more sloppy about dependencies in other ways. Not saying that that is happening here.
But e.g. not being able to use the primary source
...
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060364162)
i'm divided on this conceptually, i think with the reputation it has now, another successful attack on xz specifically is unlikely (not more likely than any other library, at least). And i'm not sure we need to go out of our way to avoid it given how much of the FOSS ecosystem relies on it.
One thing i am slightly afraid of is that in avoiding xz we become more sloppy about dependencies in other ways. Not saying that that is happening here.
But e.g. not being able to use the primary source
...
💬 laanwj commented on pull request "test: Fix failing univalue float test":
(https://github.com/bitcoin/bitcoin/pull/29892#issuecomment-2060405735)
Right. Direct floating point equality checks are a big nono. It should either use an epsilon comparison, or discretize. Which is what the new test does.
ACK fa4c69669e079c38844ecea1ad3394aae3702ae1
(https://github.com/bitcoin/bitcoin/pull/29892#issuecomment-2060405735)
Right. Direct floating point equality checks are a big nono. It should either use an epsilon comparison, or discretize. Which is what the new test does.
ACK fa4c69669e079c38844ecea1ad3394aae3702ae1
💬 maflcko commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060446429)
Not sure about the benefit to make extra steps to use a possibly non-standard, possibly easier to backdoor, mirrored tarball.
An alternative might be to completely skip the step of tarballs and instead use the upstream source control directly?
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060446429)
Not sure about the benefit to make extra steps to use a possibly non-standard, possibly easier to backdoor, mirrored tarball.
An alternative might be to completely skip the step of tarballs and instead use the upstream source control directly?
💬 maflcko commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1568304549)
> If we can rely on that to prevent race conditions then this could just be `assert_debug_log`.
`assert_debug_log` is also syncing (it has a timeout argument), so no race should happen. My comment is only about the IO usage. `assert_debug_log` is not using a busy wait, but a sleepy wait. Otherwise they are exactly identical.
However, if `assert_debug_log` is used to sync, my personal preference is to provide the `timeout` argument, so that the code is self-documenting.
I think in this c
...
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1568304549)
> If we can rely on that to prevent race conditions then this could just be `assert_debug_log`.
`assert_debug_log` is also syncing (it has a timeout argument), so no race should happen. My comment is only about the IO usage. `assert_debug_log` is not using a busy wait, but a sleepy wait. Otherwise they are exactly identical.
However, if `assert_debug_log` is used to sync, my personal preference is to provide the `timeout` argument, so that the code is self-documenting.
I think in this c
...
💬 stratospher commented on issue "Intermittent issue in p2p_handshake.py", line 75, in run_test self.test_desirable_service_flags(node, [NODE_NETWORK | NODE_WITNESS], not true after 2400.0 seconds":
(https://github.com/bitcoin/bitcoin/issues/29896#issuecomment-2060570489)
looks like this happens in `add_outbound_p2p_connection` when disconnection isn't fully over and we're trying to establish a connection again on same port since `p2p_idx=0`. will open a fix soon.
cc @theStack.
(https://github.com/bitcoin/bitcoin/issues/29896#issuecomment-2060570489)
looks like this happens in `add_outbound_p2p_connection` when disconnection isn't fully over and we're trying to establish a connection again on same port since `p2p_idx=0`. will open a fix soon.
cc @theStack.
💬 fanquake commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060615404)
> And i'm not sure we need to go out of our way to avoid it given how much of the FOSS ecosystem relies on it.
Yea. xz was a way to start a discussion. It could turn out that we actually consolidate everything to xz, and actually drop bzip2 for example.
> possibly easier to backdoor, mirrored tarball.
> and instead use the upstream source control directly
If anything, I'd hope it's less easy to backdoor, because it's just the source code, and not all the bootstrapped garbage we don't n
...
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060615404)
> And i'm not sure we need to go out of our way to avoid it given how much of the FOSS ecosystem relies on it.
Yea. xz was a way to start a discussion. It could turn out that we actually consolidate everything to xz, and actually drop bzip2 for example.
> possibly easier to backdoor, mirrored tarball.
> and instead use the upstream source control directly
If anything, I'd hope it's less easy to backdoor, because it's just the source code, and not all the bootstrapped garbage we don't n
...
💬 laanwj commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060616166)
> Not sure about the benefit to make extra steps to use a possibly non-standard, possibly easier to backdoor, mirrored tarball.
Mind that we do verify the sha256 hash. The person introducing it has to check, once, that the tarball matches exactly what is in git. i don't think there's a win in making every guix buid do a git checkout.
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060616166)
> Not sure about the benefit to make extra steps to use a possibly non-standard, possibly easier to backdoor, mirrored tarball.
Mind that we do verify the sha256 hash. The person introducing it has to check, once, that the tarball matches exactly what is in git. i don't think there's a win in making every guix buid do a git checkout.
🚀 fanquake merged a pull request: "guix: remove `gcc-toolchain static` from Windows build"
(https://github.com/bitcoin/bitcoin/pull/29828)
(https://github.com/bitcoin/bitcoin/pull/29828)
🚀 fanquake merged a pull request: "test: Fix failing univalue float test"
(https://github.com/bitcoin/bitcoin/pull/29892)
(https://github.com/bitcoin/bitcoin/pull/29892)
💬 fanquake commented on pull request "test: Fix failing univalue float test":
(https://github.com/bitcoin/bitcoin/pull/29892#issuecomment-2060642724)
Backported to 27.x in #29888.
(https://github.com/bitcoin/bitcoin/pull/29892#issuecomment-2060642724)
Backported to 27.x in #29888.