💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1650081711)
Looks like CI is now green :)
Also the test failure from https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1521387810 can now be tested on this pull with just:
```
MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh
```
Output:
```
test 2023-07-25T15:37:42.430000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
F
...
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1650081711)
Looks like CI is now green :)
Also the test failure from https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1521387810 can now be tested on this pull with just:
```
MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh
```
Output:
```
test 2023-07-25T15:37:42.430000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
F
...
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650091313)
Updated d87edf309ffe60ae044a57ce9a0d9cc711edc365 -> 6960c81cbfa6208d4098353e53b313e13a21cb49 ([kernelRmUnivalue_8](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_8) -> [kernelRmUnivalue_9](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_8..kernelRmUnivalue_9))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273714341), fixing release note.
...
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650091313)
Updated d87edf309ffe60ae044a57ce9a0d9cc711edc365 -> 6960c81cbfa6208d4098353e53b313e13a21cb49 ([kernelRmUnivalue_8](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_8) -> [kernelRmUnivalue_9](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_8..kernelRmUnivalue_9))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273714341), fixing release note.
...
💬 MarcoFalke commented on pull request "test: Drop 22.x node from TxindexCompatibilityTest":
(https://github.com/bitcoin/bitcoin/pull/28070#issuecomment-1650131175)
Dropped that change for now, to save it for a later pull.
(https://github.com/bitcoin/bitcoin/pull/28070#issuecomment-1650131175)
Dropped that change for now, to save it for a later pull.
💬 Crypt-iQ commented on pull request "test: Avoid intermittent issues due to async events in validationinterface_tests":
(https://github.com/bitcoin/bitcoin/pull/28150#issuecomment-1650137575)
> An alternative might be to call `SyncWithValidationInterfaceQueue` at the start of the test.
Tested it and both approaches work, not sure which is better. Using `ChainTestingSetup` should be faster?
(https://github.com/bitcoin/bitcoin/pull/28150#issuecomment-1650137575)
> An alternative might be to call `SyncWithValidationInterfaceQueue` at the start of the test.
Tested it and both approaches work, not sure which is better. Using `ChainTestingSetup` should be faster?
🤔 jamesob reviewed a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1541600581)
Review check-in; up to (but not including) 9e65744c4f44ce89f3176870d3cc25ac2b6bdc08
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1541600581)
Review check-in; up to (but not including) 9e65744c4f44ce89f3176870d3cc25ac2b6bdc08
💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270948739)
cacc8fe2b59e45cde248a031c5ebbf9ec39c5b8f
"thag." Sounds Nordic.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270948739)
cacc8fe2b59e45cde248a031c5ebbf9ec39c5b8f
"thag." Sounds Nordic.
💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1271064136)
df4303b996f8b4f095de18edd7d3dbe281b5f124
If you have to retouch, maybe worth keeping the `BOOST_CHECK_EQUAL` (which compiles for me okay) since it'll give a printout of the mismatch.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1271064136)
df4303b996f8b4f095de18edd7d3dbe281b5f124
If you have to retouch, maybe worth keeping the `BOOST_CHECK_EQUAL` (which compiles for me okay) since it'll give a printout of the mismatch.
💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270986511)
5e0ca4ba371570d1a5275c5a79bc0c1253d97ead
Odd to me that `KEYLEN` doesn't live higher up in the class hierarchy, since the other `ChaCha20*` only deal in 32-byte keys, but definitely not a big deal.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270986511)
5e0ca4ba371570d1a5275c5a79bc0c1253d97ead
Odd to me that `KEYLEN` doesn't live higher up in the class hierarchy, since the other `ChaCha20*` only deal in 32-byte keys, but definitely not a big deal.
📝 fanquake opened a pull request: "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds "
(https://github.com/bitcoin/bitcoin/pull/28151)
We currently work around a longstanding GCC issue with aligned vector instructions, by patching the behaviour we want into GCC (see discussion in #24736). Possibly in response to the GCC thread (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412#c40), a new option was [introduced into the binutils assembler](https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c8480b58e1968f209b6365af7422678f348222c2) with the 2.38 release:
```
x86: Add -muse-unaligned-vector-move to assembler
Unaligne
...
(https://github.com/bitcoin/bitcoin/pull/28151)
We currently work around a longstanding GCC issue with aligned vector instructions, by patching the behaviour we want into GCC (see discussion in #24736). Possibly in response to the GCC thread (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412#c40), a new option was [introduced into the binutils assembler](https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c8480b58e1968f209b6365af7422678f348222c2) with the 2.38 release:
```
x86: Add -muse-unaligned-vector-move to assembler
Unaligne
...
💬 fanquake commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1650157656)
cc @theuni
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1650157656)
cc @theuni
📝 instagibbs opened a pull request: "Add benchmark for miniminer"
(https://github.com/bitcoin/bitcoin/pull/28152)
Directly ripped off from the fuzztest
Ideally used to inform design decisions for package relay work: https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1647523520
(https://github.com/bitcoin/bitcoin/pull/28152)
Directly ripped off from the fuzztest
Ideally used to inform design decisions for package relay work: https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1647523520
👍 vasild approved a pull request: "test: fix intermittent failure in p2p_getaddr_caching.py"
(https://github.com/bitcoin/bitcoin/pull/28144#pullrequestreview-1545936696)
ACK 8a20f765cce2fc0fadf1a2b66b843b67f2d2ae12
(https://github.com/bitcoin/bitcoin/pull/28144#pullrequestreview-1545936696)
ACK 8a20f765cce2fc0fadf1a2b66b843b67f2d2ae12
💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1650184694)
new approach seems in line with what we've discussed previously. Basically https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1231092018 AJ's suggestion, minus retries, plus using explicit linearization step.
Obvious point but including miniminer dependency means we'll likely have increased review surface, even if it ends up making a lot of sense in this case. From my basic experimentation in https://github.com/bitcoin/bitcoin/pull/28152 it looks like the performance hit is negligible,
...
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1650184694)
new approach seems in line with what we've discussed previously. Basically https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1231092018 AJ's suggestion, minus retries, plus using explicit linearization step.
Obvious point but including miniminer dependency means we'll likely have increased review surface, even if it ends up making a lot of sense in this case. From my basic experimentation in https://github.com/bitcoin/bitcoin/pull/28152 it looks like the performance hit is negligible,
...
🤔 glozow reviewed a pull request: "net processing: clamp PeerManager::Options user input"
(https://github.com/bitcoin/bitcoin/pull/28149#pullrequestreview-1545932253)
utACK 128ad03792cd4aeeaf32807d07f01e3f85adaf28
Thanks for the followup
(https://github.com/bitcoin/bitcoin/pull/28149#pullrequestreview-1545932253)
utACK 128ad03792cd4aeeaf32807d07f01e3f85adaf28
Thanks for the followup
💬 glozow commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1273826506)
nit, includes a bit more than replaced and orphans
```suggestion
//! Number of non-mempool transactions to keep around for block reconstruction. Includes orphan, replaced, and rejected transactions.
```
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1273826506)
nit, includes a bit more than replaced and orphans
```suggestion
//! Number of non-mempool transactions to keep around for block reconstruction. Includes orphan, replaced, and rejected transactions.
```
💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1273836288)
@glozow fixed now using `CAmount`
Thank you
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1273836288)
@glozow fixed now using `CAmount`
Thank you
🤔 MarcoFalke reviewed a pull request: "wallet: bugfix, disallow migration of invalid scripts"
(https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1545962463)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1545962463)
Concept ACK
💬 MarcoFalke commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1273846396)
```suggestion
wallet.rpc.importaddress(address=script_sh_pkh.hex(), label="boom_script", rescan=False, p2sh=True)
```
nit: Could skip rescan, since there are no matches?
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1273846396)
```suggestion
wallet.rpc.importaddress(address=script_sh_pkh.hex(), label="boom_script", rescan=False, p2sh=True)
```
nit: Could skip rescan, since there are no matches?
👍 theuni approved a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1545977927)
Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1545977927)
Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
👍 MarcoFalke approved a pull request: "test: fix `feature_addrman.py` on big-endian systems"
(https://github.com/bitcoin/bitcoin/pull/27529#pullrequestreview-1545981390)
review lgtm. Will test later with https://github.com/bitcoin/bitcoin/pull/28087
lgtm ACK 53c990ad3406ee945305af84af98d2f020e5f316 🔚
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...
(https://github.com/bitcoin/bitcoin/pull/27529#pullrequestreview-1545981390)
review lgtm. Will test later with https://github.com/bitcoin/bitcoin/pull/28087
lgtm ACK 53c990ad3406ee945305af84af98d2f020e5f316 🔚
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...